Movable Refactoring #563
Movable Refactoring #563
Conversation
…ave games # Conflicts: # jsettlers.logic/src/main/java/jsettlers/ai/highlevel/pioneers/PioneerGroup.java # jsettlers.logic/src/main/java/jsettlers/logic/map/grid/MainGrid.java # jsettlers.logic/src/main/java/jsettlers/logic/movable/Movable.java
| * @author homoroselaps | ||
| */ | ||
|
|
||
| public class MultiMaterialComponent extends MaterialComponent { |
There was a problem hiding this comment.
Does it make sense to refactor MaterialComponent and MultiMaterialComponent together to a single class?
There was a problem hiding this comment.
The class MaterialComponent implements the function GetMaterial of the ILogicMovable interface which is used mainly for choosing the correct visuals and for Strategies (like bearer and buildingworker) assuming, that a movable carries only one material at a time. For consistency reasons with the old implementation, I chose MultiMaterialComponent to be an extension of the base assumption of "only one material per movable". (although GetMaterial becomes ambiguous for more than one material carried)
| return new Condition<>(entity -> entity.getNotificationsIt(type).hasNext()); | ||
| } | ||
|
|
||
| protected static Debug $(String message, Node<Entity> child) { |
There was a problem hiding this comment.
The name of the method does not express its behaviour.
There was a problem hiding this comment.
Thank you for your annotation. You are completely right, this indicates bad code style.
In this decision I prioritized readability of the whole behavior tree over comprehensibility of this single function. Some behavior tree frameworks ship with a graphical tool, which we don't have. Therefore, I did my best to avoid as much boilerplate code/code repetition as possible, to support behavior design in code.
The function $ is here defined as a shorthand for the creation of a ´Debug´ node, which annotates tree nodes with debug information/comments.
I choose $ as to imitate the jQuery function, which developers might already be familiar with.
I'd fine with any other valid symbol, but would rather not clatter the tree with to many
new Debug(...) statements.
fix: movables die on contact with donkeys
|
movables are getting permanently blocked when their destination is unreachable |
| protected NodeStatus onTick(Tick<T> tick) { | ||
| for (; index < children.size(); index++) { | ||
| NodeStatus status = children.get(index).execute(tick); | ||
| if (!status.equals(NodeStatus.Failure)) |
There was a problem hiding this comment.
Please always add curly brackets after ifs.
| if (note != null) { | ||
| if (consume) consumeNotification(note); | ||
| } | ||
| return null; |
There was a problem hiding this comment.
You never return a notification... even if you find one.
| return getNotificationsIt(type).hasNext(); | ||
| } | ||
|
|
||
| public <T extends Notification> Iterator<T> getNotificationsIt(Class<T> type) { |
There was a problem hiding this comment.
Some things here:
- I think it would be better to realize this with a java8 stream. From what I found, you always only check if there is a single element and then get that element. You could make this function return a Stream and build the stream like this:
return stream(entity.getAllNotifications()).filter(type::isInstance).map(notification -> (T)notification);
and when you want to get the first matching notification just do
getNotificationsIt(type).findAny().isPresent()
Furthermore, I would then rename this method to something like getNotificationsOfType.
-
Regarding the consumed status of notifcations. Have you thought about adding a consumed flag to the Notification parent class along with a
consume()method? This way you can save this state of the notification in the notification, where it belongs (more OOP). (and it should be better in performance compared to the hash set lookup). -
Is there a need to use
type.isInstance()? I mean will there be the use case that you ask for a supertype notification and expect to get a child class of it? If not, we could simply do a == comparision on the class of the notification, which should be considerably faster.
There was a problem hiding this comment.
Regarding the consumed status of notifcations.
It is important that each component is able to track which notifications got consumed so far individually. Right now the GetNotificationsOfType returns the objects from the entity. This ensures the equality of Notifications across Component borders. If the consumed flag is part of the Notification itself, then we have to override the standard equality of Notifications.
Is there a need to use type.isInstance()? I mean will there be the use case that you ask for a supertype notification and expect to get a child class of it?
Yes this is the intended behavior. This way eg a Notification GoToPostion::GoToPositionWithMode can be treated by the DefaultSteeringComponent simply as GoToPosition as well as by an SpecialSteeringComponent which supports different modes of steering.
| @Override | ||
| public short getViewDistance() { | ||
| if (!entity.containsComponent(MovableComponent.class)) return 0; | ||
| return entity.get(MovableComponent.class).getViewDistance(); |
There was a problem hiding this comment.
I would change these methods to directly do the get on entity and then check if the result is null. If so, return the default value, if it isn't null, call the method. This way, the code does not need to query the set twice.
| )); | ||
| } | ||
|
|
||
| private static Action<Context> accept_SaveDeliveryJob() { |
There was a problem hiding this comment.
Please don't use underscores in Java method names. Camel case is the way to go. Furthermore, accept_Save seems kind of doubled.
|
@homoroselaps: I really like the idea of using behavior trees! It really offers a way to do more complex stuff in a much cleaner way. However, I was wondering, isn't their a working library out there already that we could use? I think if such a library does not have major drawbacks, it's probably better to use it off the shelf instead of reimplementing it. Do you know about such libaries? |
…enable debugging of an entity via the "d" key used on selected enitites
|
@homoroselaps: It would be great, if you could reach out to me on Discord so I can ask you some questions about your code. |
…important tree node factories
# Conflicts: # jsettlers.buildingcreator/src/main/java/jsettlers/buildingcreator/job/EBuildingJobType.java # jsettlers.logic/src/main/java/jsettlers/algorithms/fogofwar/FogOfWar.java # jsettlers.logic/src/main/java/jsettlers/input/GuiTaskExecutor.java # jsettlers.logic/src/main/java/jsettlers/logic/map/grid/GameSerializer.java # jsettlers.logic/src/main/java/jsettlers/logic/map/grid/MainGrid.java # jsettlers.logic/src/main/java/jsettlers/logic/movable/Movable.java # jsettlers.main.android/src/main/java/jsettlers/main/android/gameplay/controlsmenu/selection/features/DestroyFeature.java # jsettlers.main.swing/src/main/java/jsettlers/main/swing/SwingManagedJSettlers.java # jsettlers.main.swing/src/main/java/jsettlers/main/swing/settings/SettingsManager.java # jsettlers.testutils/src/main/resources/jsettlers/integration/replay/fullproduction/savegame-10m.zmap
…pBuilder, Lumberjack, Toolsmith, Melter)
This refactoring includes:
The new architecture is meant to replace implementations of all movables.
For code exploration of the ECM you might use
EntityFactory.CreateMovable()as a starting point.For code exploration of the behavior tree you might use
DonkeyBehaviorComponent.