-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feature(ecs-gestalt): Migrate Components to gestalt's Components. #4753
Conversation
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.
I like that you try to push adoption here now that we have the foundation based on gestalt v7 👍
As mentioned on Discord, I'm looking at the code right now and I'm wondering about the "deep copy" advise given in the gestalt wiki.
We have lots of components holding collections of elements, and in this first iteration you only do a shallow clone. Is that intentional?
I'm concerned that, if we don't have best-practice implementations in the engine and in Iota, we'll have a hard time fixing those up in the aftermath.
Also, we'll need to update all modules in Omega to not break the workspace. I think we wanted to throw out a couple of modules that are not working anymore, which will at least reduce the scope slightly...
@@ -165,7 +166,7 @@ private static EventSystem createEventSystem(NetworkSystem networkSystem, PojoEn | |||
|
|||
private static void registerComponents(ComponentLibrary library, ModuleEnvironment environment) { | |||
for (Class<? extends Component> componentType : environment.getSubtypesOf(Component.class)) { | |||
if (componentType.getAnnotation(DoNotAutoRegister.class) == null) { | |||
if (componentType.getAnnotation(DoNotAutoRegister.class) == null && !componentType.isInterface() && !Modifier.isAbstract(componentType.getModifiers())) { |
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.
why is this change necessary?
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.
Because:
@DoNotAutoRegister
is part of TS, not gestalt.- It fails with
EmptyComponent
. really abstract classes is not functional for ECS.
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.
Long line warning, add line break.
engine-tests/src/main/java/org/terasology/unittest/stubs/ListOfEnumsComponent.java
Outdated
Show resolved
Hide resolved
Provide shallow and deep copy for collections. |
Ref: MovingBlocks/Terasology#4753 Co-authored-by: Michael Pollind <mpollind@gmail.com> Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>
Ref: MovingBlocks/Terasology#4753 Co-authored-by: Michael Pollind <mpollind@gmail.com> Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>
Ref: MovingBlocks/Terasology#4753 Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>
Ref: MovingBlocks/Terasology#4753 Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>
Ref: MovingBlocks/Terasology#4753 Co-authored-by: Michael Pollind <mpollind@gmail.com> Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>
Ref: MovingBlocks/Terasology#4753 Co-authored-by: Michael Pollind <mpollind@gmail.com> Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>
Ref: MovingBlocks/Terasology#4753 Co-authored-by: Michael Pollind <mpollind@gmail.com> Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>
Ref: MovingBlocks/Terasology#4753 Co-authored-by: Michael Pollind <mpollind@gmail.com> Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>
Ref: MovingBlocks/Terasology#4753 Co-authored-by: Michael Pollind <mpollind@gmail.com> Co-authored-by: Tobias Nett <skaldarnar@googlemail.com> Co-authored-by: jdrueckert <jd.rueckert@googlemail.com>
Ref: MovingBlocks/Terasology#4753 Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>
ref: MovingBlocks/Terasology#4753 Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>
Contains
This is the first step of re-integration gestalt's Entity System (
gestalt-entity-system
) to Terasology by switching to use gestalt'sComponent
interface.This PR does not yet introduce any entity system logic from gestalt, it just switches out Terasology's
Component
marker interface for the one from gestalt.As gestalt assumes that
Component#copyFrom
is doing a deep copy, a potential bug is non-cloning collections and nested types properly at component data copy. Since we're not yet using this functionality we have to keep this pitfall in mind once we're switching to usegestalt-entity-system
completely.How to test
All branches for this migration PR are named
feature/migrate-ecs-to-gestalt
.gradlew compileJava
gradlew compileTestJava
gradlew test
Need ToDo
Component::copy
toComponent::copyFrom
(#122) gestalt#123