fix: resolve issue when loading a world a second time #4829

pollend · 2021-07-25T03:30:59Z

looks like gestalt loads a whole new environment so any assets from one session are not compatible for another session. there are two main things I do, I wipe all the prefabs and let the engine re-resolve them when the world loads and unload any assets that are not loaded in by core.

pollend · 2021-07-25T03:34:08Z

engine/src/main/java/org/terasology/engine/core/bootstrap/EnvironmentSwitchHandler.java

+        assetTypeManager.getAssetTypes().forEach(k -> {
+            for (ResourceUrn urn : k.getLoadedAssetUrns()) {
+                if (!urn.getModuleName().equals(TerasologyConstants.ENGINE_MODULE)) {
+                    k.getAsset(urn).ifPresent(Asset::dispose);
+                }
+            }
+        });
+        assetTypeManager.getAssetType(BlockFamilyDefinition.class).ifPresent(AssetType::disposeAll);
+        assetTypeManager.getAssetType(Prefab.class).ifPresent(AssetType::disposeAll);


umm must be a better way to clean this up 👎

keturn

I think my main question here is: can we run the disposal code when we switch out of the game environment? Wouldn't that be a better match to the expected lifecycle of these assets?

engine/src/main/java/org/terasology/engine/core/modes/StateLoading.java

keturn · 2021-07-25T21:14:24Z

engine/src/main/java/org/terasology/engine/core/modes/loadProcesses/InitialiseRendering.java

+
+    @Override
+    public boolean step() {
+        context.put(RenderingModuleRegistry.class, new RenderingModuleRegistry());


What makes this better as a LoadProcess instead of a Subsystem?

more explicit ordering of load processes? or we need a different context here than was available for any of the subsystem lifecycle phases?

(and heck it is a lot of lines of code for these two expressions, either way)

we need a new rendering system when you restart a world. rendering fails to init correctly when its already been constructed.

For my information: Why do we need a new rendering system when restarting a world?

its all module drive there really isn't a reason to keep it around as a singleton. just means we have to manage the lifecycle at a much higher level. introduces extra book keeping that I would like to avoid. anyways one session might not have the same rendering setup so the entire state of the RenderingModuleRegistry would need to be wiped. just easier to dispose of the whole object.

yea.. we do this for quite a few loading things.

Can we please persist this information in a docstring. Probably the class doc here is a good place to capture this.

jdrueckert · 2021-07-26T20:21:36Z

engine/src/main/java/org/terasology/engine/core/bootstrap/EnvironmentSwitchHandler.java

        unregisterPrefabFormats(assetTypeManager);
+        assetTypeManager.getAssetTypes().forEach(k -> {
+            for (ResourceUrn urn : k.getLoadedAssetUrns()) {
+                if (!urn.getModuleName().equals(TerasologyConstants.ENGINE_MODULE)) {
+                    k.getAsset(urn).ifPresent(Asset::dispose);
+                }
+            }
+        });


Why do we unregister prefab formats and dispose assets at this point?
Shouldn't this be done when exiting the previous world rather than when entering the next? 🤔
(same for asset disposal below)

Same question as #4829 (review) 🙃

we end up loading those classes again on the classpath so the prefabs are not compatible between sessions.

jdrueckert · 2021-07-27T20:52:57Z

Second World Start Bug

some changes

…oad-a-world-second-time

…fix/resolve-issue-when-attempting-to-load-a-world-second-time

jdrueckert · 2021-08-03T19:16:25Z

I tested this out and it definitely is not crashing anymore, neither when loading nor when starting a second or third world! 🎉

However... I saw the following, so I assume we're still not cleaning up everything properly?

I was in my LaS workspace (because testing some other things before) and did the following:

Start a new LaS world
Load an older LaS world
Start a new Core Gameplay world

pollend · 2021-08-04T02:18:53Z

this only broke after the tinting was added I'm sure its a couple line fix but I thought it was out of the scope of this change. @jdrueckert I was planning on looking at this separately. I've addressed the changes that @keturn raised about clean up so these changes are good from that standpoint.

…oad-a-world-second-time

jdrueckert

Tests out fine now and I don't see anything in the code that worries me. However, I'd like @keturn and / or @skaldarnar to have a final look, too, so I won't approve this yet.
@keturn, @skaldarnar feel free to approve and merge after you've had a look in case you're good with the changes.

engine/src/main/java/org/terasology/engine/core/bootstrap/EnvironmentSwitchHandler.java

engine/src/main/java/org/terasology/engine/core/modes/StateIngame.java

skaldarnar · 2021-08-07T11:41:16Z

engine/src/main/java/org/terasology/engine/core/modes/StateIngame.java

        chunkProvider.dispose();

+        assetTypeManager.getAssetTypes().forEach(k -> {


What is k supposed to mean here? Can we please name it assetType?

Suggested change

assetTypeManager.getAssetTypes().forEach(k -> {

// dispose all module assets

assetTypeManager.getAssetTypes().forEach(k -> {

engine/src/main/java/org/terasology/engine/core/modes/StateIngame.java

skaldarnar · 2021-08-07T11:48:03Z

engine/src/main/java/org/terasology/engine/world/block/DefaultColorSource.java

@@ -17,29 +17,23 @@ public Colorc calcColor(int x, int y, int z) {
    COLOR_LUT {
        @Override
        public Colorc calcColor(int x, int y, int z) {
+            ColorProvider colorProvider = CoreRegistry.get(ColorProvider.class);


Is doing this lookup in CoreRegistry fast? I think @tolziplohu put this optimization on purpose to reduce the necessary lookups in the registry with each lookup of a color... 🤔

yea, but we can't save it as a static variable.

skaldarnar · 2021-08-07T11:48:39Z

engine/src/main/java/org/terasology/engine/world/block/loader/BlockFamilyDefinitionFormat.java

@@ -318,7 +318,6 @@ private BlockFamilyDefinitionData createBaseData(JsonObject jsonObject) {
        @Override
        public Class<? extends BlockFamily> deserialize(JsonElement json, Type typeOfT, JsonDeserializationContext context)
                throws JsonParseException {
-


All these changes are unrelated to this PR. Please revert them.

skaldarnar · 2021-08-07T11:49:49Z

engine/src/main/java/org/terasology/engine/core/modes/loadProcesses/InitialiseRendering.java

+
+    @Override
+    public boolean step() {
+        context.put(RenderingModuleRegistry.class, new RenderingModuleRegistry());


Can we please persist this information in a docstring. Probably the class doc here is a good place to capture this.

…ronmentSwitchHandler.java Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>

…ame.java Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>

engine/src/main/java/org/terasology/engine/core/modes/loadProcesses/InitialiseRendering.java

engine/src/main/java/org/terasology/engine/core/modes/StateIngame.java

skaldarnar · 2021-08-07T21:43:25Z

engine/src/main/java/org/terasology/engine/core/modes/StateIngame.java

        chunkProvider.dispose();

+        assetTypeManager.getAssetTypes().forEach(k -> {


Suggested change

assetTypeManager.getAssetTypes().forEach(k -> {

// dispose all module assets

assetTypeManager.getAssetTypes().forEach(k -> {

engine/src/main/java/org/terasology/engine/core/modes/StateIngame.java

…ame.java Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>

…esses/InitialiseRendering.java Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>

bugfix: resolved loading issue when attempting to start a world twice

79c0336

github-actions bot added the Type: Bug Issues reporting and PRs fixing problems label Jul 25, 2021

chore: cleanup

ff8608a

pollend commented Jul 25, 2021

View reviewed changes

chore: add InitialiseRendering

da58e0c

pollend requested review from keturn and skaldarnar July 25, 2021 15:38

keturn suggested changes Jul 25, 2021

View reviewed changes

jdrueckert reviewed Jul 26, 2021

View reviewed changes

chore: add rendering to client and only how with a local client

d69a0d7

jdrueckert mentioned this pull request Jul 27, 2021

Crash when starting/loading/joining a second world #4832

Closed

jdrueckert added this to the v5.1.0 milestone Jul 27, 2021

keturn self-requested a review July 27, 2021 22:10

keturn previously approved these changes Jul 27, 2021

View reviewed changes

pollend and others added 2 commits July 27, 2021 15:21

Merge branch 'develop' into bugfix/resolve-issue-when-attempting-to-l…

ca69dcf

…oad-a-world-second-time

Merge branch 'develop' of github.com:MovingBlocks/Terasology into bug…

77254ed

…fix/resolve-issue-when-attempting-to-load-a-world-second-time

pollend requested review from keturn and jdrueckert August 3, 2021 02:31

pollend and others added 2 commits August 3, 2021 19:18

Merge branch 'develop' into bugfix/resolve-issue-when-attempting-to-l…

f8f003c

…oad-a-world-second-time

chore: remove caching from DefaultColorSource

c2403f4

jdrueckert reviewed Aug 5, 2021

View reviewed changes

skaldarnar requested changes Aug 7, 2021

View reviewed changes

Update engine/src/main/java/org/terasology/engine/core/bootstrap/Envi…

029062f

…ronmentSwitchHandler.java Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>

pollend and others added 4 commits August 7, 2021 09:10

Update engine/src/main/java/org/terasology/engine/core/modes/StateIng…

5be727a

…ame.java Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>

Update engine/src/main/java/org/terasology/engine/core/modes/StateIng…

892bc70

…ame.java Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>

Update engine/src/main/java/org/terasology/engine/core/modes/StateIng…

d5b173f

…ame.java Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>

chore: apply suggestion

d0a3f46

pollend requested a review from skaldarnar August 7, 2021 18:22

skaldarnar reviewed Aug 7, 2021

View reviewed changes

pollend and others added 4 commits August 7, 2021 15:30

Update engine/src/main/java/org/terasology/engine/core/modes/StateIng…

9bac248

…ame.java Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>

Update engine/src/main/java/org/terasology/engine/core/modes/StateIng…

48ee146

…ame.java Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>

Update engine/src/main/java/org/terasology/engine/core/modes/loadProc…

e890e22

…esses/InitialiseRendering.java Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>

chore: add missing comment

6d97df3

skaldarnar approved these changes Aug 8, 2021

View reviewed changes

skaldarnar changed the title ~~bugfix: resolved loading issue when attempting to start a world twice~~ fix: resolve issue when loading a world a second time Aug 8, 2021

skaldarnar merged commit 050fd8d into develop Aug 8, 2021

skaldarnar deleted the bugfix/resolve-issue-when-attempting-to-load-a-world-second-time branch August 8, 2021 10:17

keturn mentioned this pull request Aug 8, 2021

MTE's own test suite is very slow Terasology/ModuleTestingEnvironment#49

Closed

pollend mentioned this pull request Aug 12, 2021

fix: resolve Advance World Settings issue #4840

Merged

pollend mentioned this pull request Aug 22, 2021

Fatal "Component not registered" error on creating a second world #4716

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: resolve issue when loading a world a second time #4829

fix: resolve issue when loading a world a second time #4829

pollend commented Jul 25, 2021

pollend Jul 25, 2021

keturn left a comment

keturn Jul 25, 2021

pollend Jul 26, 2021 •

edited

Loading

jdrueckert Jul 26, 2021

pollend Jul 26, 2021 •

edited

Loading

jdrueckert Jul 27, 2021

pollend Jul 28, 2021

skaldarnar Aug 7, 2021

jdrueckert Jul 26, 2021 •

edited

Loading

skaldarnar Jul 26, 2021

pollend Jul 28, 2021

jdrueckert commented Jul 27, 2021

jdrueckert commented Aug 3, 2021

pollend commented Aug 4, 2021 •

edited

Loading

jdrueckert left a comment

skaldarnar Aug 7, 2021

skaldarnar Aug 7, 2021

skaldarnar Aug 7, 2021

pollend Aug 7, 2021

skaldarnar Aug 7, 2021

skaldarnar Aug 7, 2021

skaldarnar Aug 7, 2021

		chunkProvider.dispose();

		assetTypeManager.getAssetTypes().forEach(k -> {

	assetTypeManager.getAssetTypes().forEach(k -> {
	// dispose all module assets
	assetTypeManager.getAssetTypes().forEach(k -> {

fix: resolve issue when loading a world a second time #4829

fix: resolve issue when loading a world a second time #4829

Conversation

pollend commented Jul 25, 2021

Choose a reason for hiding this comment

keturn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pollend Jul 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pollend Jul 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdrueckert Jul 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdrueckert commented Jul 27, 2021

jdrueckert commented Aug 3, 2021

pollend commented Aug 4, 2021 • edited Loading

jdrueckert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pollend Jul 26, 2021 •

edited

Loading

pollend Jul 26, 2021 •

edited

Loading

jdrueckert Jul 26, 2021 •

edited

Loading

pollend commented Aug 4, 2021 •

edited

Loading