-
-
Notifications
You must be signed in to change notification settings - Fork 367
Colony expedition system #10286
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: version/main
Are you sure you want to change the base?
Colony expedition system #10286
Conversation
# Conflicts: # src/api/java/com/minecolonies/api/colony/IVisitorData.java # src/main/java/com/minecolonies/apiimp/initializer/EntityInitializer.java # src/main/java/com/minecolonies/core/MineColonies.java # src/main/java/com/minecolonies/core/colony/interactionhandling/RecruitmentInteraction.java # src/main/java/com/minecolonies/core/colony/managers/CitizenManager.java # src/main/java/com/minecolonies/core/colony/managers/VisitorManager.java # src/main/java/com/minecolonies/core/datalistener/CustomVisitorListener.java # src/main/java/com/minecolonies/core/entity/ai/visitor/EntityAIVisitor.java # src/main/java/com/minecolonies/core/entity/citizen/VisitorCitizen.java
# Conflicts: # src/main/java/com/minecolonies/api/IMinecoloniesAPI.java # src/main/java/com/minecolonies/api/MinecoloniesAPIProxy.java # src/main/java/com/minecolonies/apiimp/CommonMinecoloniesAPIImpl.java # src/main/java/com/minecolonies/apiimp/initializer/EntityInitializer.java # src/main/java/com/minecolonies/core/entity/visitor/VisitorCitizen.java
# Conflicts: # src/main/java/com/minecolonies/api/colony/managers/interfaces/IExpeditionManager.java # src/main/java/com/minecolonies/core/colony/managers/VisitorManager.java
# Conflicts: # src/main/java/com/minecolonies/core/entity/visitor/VisitorCitizen.java
# Conflicts: # src/main/java/com/minecolonies/core/entity/visitor/VisitorCitizen.java
# Conflicts: # gradle.properties
# Conflicts: # gradle.properties # src/main/java/com/minecolonies/core/colony/Colony.java # src/main/java/com/minecolonies/core/entity/visitor/VisitorCitizen.java # src/main/java/com/minecolonies/core/event/DataPackSyncEventHandler.java # src/main/java/com/minecolonies/core/event/EventHandler.java # src/main/java/com/minecolonies/core/event/FMLEventHandler.java # src/main/java/com/minecolonies/core/network/NetworkChannel.java # src/main/resources/assets/minecolonies/lang/manual_en_us.json
src/main/java/com/minecolonies/api/colony/managers/interfaces/expeditions/ColonyExpedition.java
Outdated
Show resolved
Hide resolved
# Conflicts: # src/main/java/com/minecolonies/api/colony/ICitizenDataView.java # src/main/java/com/minecolonies/api/entity/ModEntities.java # src/main/java/com/minecolonies/apiimp/initializer/EntityInitializer.java # src/main/java/com/minecolonies/core/entity/ai/visitor/EntityAIVisitor.java # src/main/java/com/minecolonies/core/entity/visitor/VisitorCitizen.java # src/main/resources/assets/minecolonies/lang/manual_en_us.json
# Conflicts: # src/main/java/com/minecolonies/api/loot/ModLootConditions.java
# Conflicts: # src/main/java/com/minecolonies/core/colony/ColonyView.java # src/main/resources/assets/minecolonies/lang/manual_en_us.json
# Conflicts: # gradle.properties
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so I'm half way through. I hopefully will do some more tomorrow.
I left a bunch of comments on the way.
Generally, the PR description needs a lot more info:
a) UIs
b) Description of expected input cost of expeditions
c) Expected outcome (death/successrate/rewards)
d) Generic description how an expedition works and how the player workflow looks like.
...a/com/minecolonies/api/entity/ai/statemachine/tickratestatemachine/TickRateStateMachine.java
Show resolved
Hide resolved
src/main/java/com/minecolonies/api/equipment/ModEquipmentTypes.java
Outdated
Show resolved
Hide resolved
/** | ||
* Gui for viewing past expeditions. | ||
*/ | ||
public class WindowTownHallExpeditions extends AbstractWindowSkeleton implements ButtonHandler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a good review I think we need these UIs with example screenshots in the PR description.
The UI code is very simple, but I think it's important to see how it all looks like to give proper feedback.
src/main/java/com/minecolonies/core/colony/events/ColonyExpeditionEvent.java
Outdated
Show resolved
Hide resolved
* @param damage the amount of damage dealt. | ||
* @param onBreak function called when an armor slot breaks. | ||
*/ | ||
private void damageArmor(final @NotNull ArmorList list, final float damage, final Consumer<EquipmentSlot> onBreak) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generic Comment: Have you tested complete client quit and restart mid expedition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's able to properly continue mid way as all necessary pieces of information are stored in NBT of the event.
} | ||
|
||
@Override | ||
public ItemStack getDefaultItemStack() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems very bad performance wise. What is this for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only utilized upon transferring the items into the expedition sheet when in creative. It'll apply a default item stack to insert given the user will have nothing in their inventory at the time.
(This is also mentioned in the interface javadoc what it does)
src/main/java/com/minecolonies/core/colony/expeditions/encounters/ExpeditionEncounter.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay second and final review (got fully through now).
Second part looks pretty okay as well, mostly a lot of streams we should avoid with small number of values and where things can break.
A thing that I think is missing is that we have to avoid citizens from being fired from their job while on an expedition (like you can't fire a guard when on expedition).
src/main/java/com/minecolonies/core/colony/managers/TravelingManager.java
Outdated
Show resolved
Hide resolved
src/main/java/com/minecolonies/core/colony/managers/TravelingManager.java
Outdated
Show resolved
Hide resolved
src/main/java/com/minecolonies/core/colony/managers/TravelingManager.java
Outdated
Show resolved
Hide resolved
src/main/java/com/minecolonies/core/colony/managers/TravelingManager.java
Outdated
Show resolved
Hide resolved
src/main/java/com/minecolonies/core/entity/ai/visitor/EntityAIExpeditionary.java
Outdated
Show resolved
Hide resolved
src/main/java/com/minecolonies/core/entity/visitor/ExpeditionaryVisitorType.java
Outdated
Show resolved
Hide resolved
...colonies/core/network/messages/server/colony/visitor/expeditionary/TransferItemsMessage.java
Outdated
Show resolved
Hide resolved
# Conflicts: # gradle.properties # src/main/java/com/minecolonies/api/colony/ICitizenDataView.java # src/main/java/com/minecolonies/apiimp/initializer/ModItemsInitializer.java # src/main/java/com/minecolonies/core/colony/CitizenData.java # src/main/java/com/minecolonies/core/colony/ColonyView.java # src/main/java/com/minecolonies/core/colony/buildings/modules/BuildingResourcesModule.java # src/main/java/com/minecolonies/core/colony/buildings/modules/TavernBuildingModule.java # src/main/java/com/minecolonies/core/event/EventHandler.java # src/main/java/com/minecolonies/core/event/FMLEventHandler.java # src/main/resources/assets/minecolonies/gui/layouthuts/layoutbuilderres.xml # src/main/resources/assets/minecolonies/gui/layouthuts/layoutwarehouseoptions.xml # src/main/resources/assets/minecolonies/gui/townhall/layoutactions.xml # src/main/resources/assets/minecolonies/lang/manual_en_us.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some more comments
@@ -232,4 +233,25 @@ public static boolean canPerformDefaultActions(ItemStack itemStack, Set<ToolActi | |||
} | |||
return true; | |||
} | |||
|
|||
@NotNull | |||
public static EquipmentTypeEntry[] getAllWeapons() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Atm, we only allow swords and bows for weapons, though right.
@@ -55,8 +55,13 @@ public void setItems(@NotNull final List<ItemStack> items) | |||
throw new IllegalArgumentException("Items list must contain at least one item."); | |||
} | |||
|
|||
final boolean hasChanged = !this.items.equals(items); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I'd just make sure the index doesn't exceed list size and otherwise don't reset it on setItems?
Changes proposed in this pull request:
You will have to give them a given amount of goods and assign one or more guards to travel alongside them.
The process and some screenshots.
You start of by getting visitors spawn in your town hall, there is no research or any kind of requirement for this to start happening. These can be identified by the map icon above their heads.
Talking to them will result in them asking if you want to go on an expedition, you can accept or deny.
You will then receive the expedition guide scroll, from here you can manage the inventory for the expedition as well as which guards are assigned.
After providing them with all the stuff, you return to the expeditionary, and attempt to start the expedition, if any requirement isn't met they will tell you, otherwise they will prompt to start the expedition.
Initially the expeditionary will stick around for a bit whilst they start packing up (in future release this will involve walking to the gate). If you attempt to talk to them at this time you'll receive the following prompt (does nothing).
When they return you can claim the loot from the expedition, and you can then see the past 5 expeditions in the townhall expeditions overview.
[X] Yes I tested this before submitting it.
[ ] I also did a multiplayer test.
Review please