Skip to content
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

Merged
merged 27 commits into from
Aug 25, 2021

Conversation

DarkWeird
Copy link
Contributor

@DarkWeird DarkWeird commented Jun 9, 2021

Contains

This is the first step of re-integration gestalt's Entity System (gestalt-entity-system) to Terasology by switching to use gestalt's Component 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 use gestalt-entity-system completely.

How to test

All branches for this migration PR are named feature/migrate-ecs-to-gestalt.

for m in . $(ls -d modules/*/); do                                                                                                                    
	git -C $m remote update -p
	git -C $m switch feature/migrate-ecs-to-gestalt
	git -C $m merge --ff-only @{u}
done
  • Compile (Ωmega workspace)
    • gradlew compileJava
    • gradlew compileTestJava
  • Run Game
  • Start Game
  • Run Tests
    • gradlew test

Need ToDo

Copy link
Member

@skaldarnar skaldarnar left a 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())) {
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because:

  1. @DoNotAutoRegister is part of TS, not gestalt.
  2. It fails with EmptyComponent. really abstract classes is not functional for ECS.

Copy link
Member

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.

@DarkWeird
Copy link
Contributor Author

Provide shallow and deep copy for collections.
shallow-copy for collections with immutable copies (String, primitives, enums, EntityRef,Assets,AssetData,Block... and custom immutable types)
deep-copy for several collections, where mutable types is using.

jdrueckert pushed a commit to Terasology/ManualLaborEventualSkills that referenced this pull request Aug 25, 2021
Ref: MovingBlocks/Terasology#4753

Co-authored-by: Michael Pollind <mpollind@gmail.com>
Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>
jdrueckert pushed a commit to Terasology/EquipmentSmithing that referenced this pull request Aug 25, 2021
Ref: MovingBlocks/Terasology#4753

Co-authored-by: Michael Pollind <mpollind@gmail.com>
Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>
jdrueckert pushed a commit to Terasology/Alchemy that referenced this pull request Aug 25, 2021
jdrueckert pushed a commit to Terasology/LightAndShadow that referenced this pull request Aug 25, 2021
jdrueckert pushed a commit to Terasology/GooeyDefence that referenced this pull request Aug 25, 2021
Ref: MovingBlocks/Terasology#4753

Co-authored-by: Michael Pollind <mpollind@gmail.com>
Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>
jdrueckert pushed a commit to Terasology/IRLCorp that referenced this pull request Aug 25, 2021
Ref: MovingBlocks/Terasology#4753

Co-authored-by: Michael Pollind <mpollind@gmail.com>
Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>
jdrueckert pushed a commit to Terasology/GooeysQuests that referenced this pull request Aug 25, 2021
Ref: MovingBlocks/Terasology#4753

Co-authored-by: Michael Pollind <mpollind@gmail.com>
Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>
jdrueckert pushed a commit to Terasology/JoshariasSurvival that referenced this pull request Aug 25, 2021
Ref: MovingBlocks/Terasology#4753

Co-authored-by: Michael Pollind <mpollind@gmail.com>
Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>
jdrueckert added a commit to Terasology/MetalRenegades that referenced this pull request Aug 25, 2021
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>
jdrueckert pushed a commit to Terasology/AlchemyPlantGenerator that referenced this pull request Aug 26, 2021
skaldarnar added a commit to Terasology/Xmas that referenced this pull request Aug 28, 2021
@keturn keturn added this to the v5.2.0 milestone Sep 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change API breaking change requiring follow-up work in dependant areas Topic: Architecture Requests, Issues and Changes related to software architecture, programming patterns, etc. Type: Improvement Request for or addition/enhancement of a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants