Merge world config screen into preview screen #1726

msteiger · 2015-05-14T16:09:43Z

This PR merges the Config and the Preview screen into one, allowing for interactive changes:

It also contains a few related fixes and features:

Multi-threaded computation for preview images
Re-use ByteBuffers to avoid garbage collection
Fix the TempWorldGeneratorPluginLibrary (issue Crash on "preview" or "config" buttons on create world screen, TTA active #1524) - TtA still crashes, but for a different reason and will be fixed in a follow-up.
Support changing seed values (without re-creating the entire world generator)

However, it

changes the WorldConfigurator interface
changes the WorldBuilderclass

This requires fixes in all modules with world generators 😢

Cervator · 2015-05-15T04:50:11Z

Looking forward to playing with this in the weekend!

I'm not too worried about changing a few world-bearing modules, that's not a big deal :-)

However, at least in the case of WorldConfigurator.java, couldn't we just take advantage of Java 8 and provide a default interface method? We could even replace WorldConfiguratorAdapter that way entirely if I'm not mistaken?

Which I may well be - taking a while to get used to the new stuff :D

There are some other (unrelated) places we can do that too, I believe.

flo · 2015-05-16T12:01:02Z

engine/src/main/java/org/terasology/rendering/nui/layers/mainMenu/PreviewWorldScreen.java

            ResolutionResult result = resolver.resolve(moduleName);
            if (result.isSuccess()) {
                environment = moduleManager.loadEnvironment(result.getModules(), false);
+                CoreRegistry.put(WorldGeneratorPluginLibrary.class, new TempWorldGeneratorPluginLibrary(environment));
+                CoreRegistry.get(AssetManager.class).setEnvironment(environment);


I am currently trying to remove CoreRegistry usage. The AssetManager can also be obtained via depenendcy injection. e.g. by adding a private AssetManager field with an @in annotation.

The registration can be done after #1721 is merged by obtaining a Context instance via dependency injection. It offers a put method like CoreRegistry. Although I find a bit strange of a location for registering objects.

flo · 2015-05-16T12:16:20Z

@msteiger I tried it out, it's a cool feature. Do you plan to make it recalculate automatically at every value change? That would be even cooler 8)

Also a randomize button next to the seen value would be qutie useful. Maybe we could call it "Next" to keep it simpler

Changing the high map caused some exceptions to be logged, but game didn't crash:
http://paste.ubuntu.com/11165495/

msteiger · 2015-05-17T15:54:14Z

Thanks for the review. I will wait until #1721 is resolved and update this PR accordingly. Functional changes/improvements will be in the next PR :-)

@Cervator I don't know what's best here, so I tend to stick with well-approved approaches. Also there are some issues with default methods to keep in mind.

Cervator · 2015-05-17T18:18:08Z

Merge of #1721 is complete, update away :-)

Thanks for the article link, interesting content and good things to watch out for. I think we're pretty safe here, as long as we don't go crazy. Want to try throwing away the old adapter class?

Also take a look at PolyWorld with the PR just merged, there was a broken (as in, fail to compile) unit test in there after the merge, IIRC (you saw it earlier, unsure if there's a fix already waiting)

GooeyHub · 2015-05-18T16:11:03Z

Refer to this link for build results (access rights to CI server needed):
http://jenkins.terasology.org/job/TerasologyPRs/82/
Hooray Jenkins reported success with all tests good!

GooeyHub · 2015-05-18T16:32:42Z

Refer to this link for build results (access rights to CI server needed):
http://jenkins.terasology.org/job/TerasologyPRs/83/
Hooray Jenkins reported success with all tests good!

msteiger · 2015-05-18T17:20:12Z

I've updated the code and rebased it on top of develop.

It still uses CoreRegistry, since this is where WorldBuilder is looking for plugins. I'm not sure how this could be changed reasonably as it would require injecting different Context instances (as I understand it). In any case I consider this out of scope for this PR and can be dealt with separately.

Cervator · 2015-05-19T05:31:51Z

Looked at this some more. A few of the module worlds are easy fixes (just remove the seed parameter from createWorld right?) but a few others are more tricky.

Cities, LightAndShadow, and Pathfinding have their world gens extend AbstractBaseWorldGenerator, which now means they require getConfigurator. Yet if I add that and just return null it would seem badness can happen in PreviewWorldScreen.configureProperties, and a few other places in that class have NPE warnings in IntelliJ

Is that class just obsolete now or something? Seemingly used in CreateGameScreen, which also has NPE warnings.

WoodAndStoneWorldGenerator extends PluggableWorldGenerator and has a getConfigurator, but that now a wrong return type. That was the main place I saw I could just return null but that would probably lead to NPEs

@msteiger want to add some more NPE hardening and/or merge this then apply fixes to the modules directly next? Two trivial fixes in AnotherWorld + TutorialWorldGeneration, same fix in Pathfinding but it also needs more.

I couldn't replicate the same errors @flo reported in the HeightMap world, although I did crash on shutdown:

01:26:36.111 [main] INFO  o.terasology.engine.TerasologyEngine - Shutting down Terasology...
01:26:36.371 [main] INFO  o.t.p.i.ReadWriteStorageManager - Saving - Creating game snapshot
01:26:36.392 [main] INFO  o.t.p.i.ReadWriteStorageManager - Saving - Snapshot created: Writing phase starts
01:26:36.394 [main] INFO  o.t.n.internal.NetworkSystemImpl - Client disconnected: Cervator
01:26:36.408 [main] INFO  o.t.logic.console.ConsoleImpl - [NOTIFICATION] Player "Cervator" has left the game
01:26:36.411 [main] INFO  o.t.n.internal.NetworkSystemImpl - Network shutdown
01:26:36.599 [Chunk-Updater-0] ERROR o.t.u.concurrency.TaskProcessor - Error in thread Chunk-Updater-0
java.lang.NullPointerException: null
    at org.terasology.world.chunks.internal.ChunkImpl.getSunlight(ChunkImpl.java:206) ~[classes/:na]
    at org.terasology.world.chunks.internal.ChunkImpl.getSunlight(ChunkImpl.java:201) ~[classes/:na]
    at org.terasology.world.internal.ChunkViewCoreImpl.getSunlight(ChunkViewCoreImpl.java:138) ~[classes/:na]
    at org.terasology.world.internal.ChunkViewCoreImpl.getSunlight(ChunkViewCoreImpl.java:113) ~[classes/:na]
    at org.terasology.rendering.primitives.ChunkTessellator.calcLightingValuesForVertexPos(ChunkTessellator.java:179) ~[classes/:na]
    at org.terasology.rendering.primitives.ChunkTessellator.generateOptimizedBuffers(ChunkTessellator.java:116) ~[classes/:na]
    at org.terasology.rendering.primitives.ChunkTessellator.generateMesh(ChunkTessellator.java:69) ~[classes/:na]
    at org.terasology.rendering.world.ChunkMeshUpdateManager$ChunkUpdateTask.run(ChunkMeshUpdateManager.java:169) ~[classes/:na]
    at org.terasology.utilities.concurrency.TaskProcessor.run(TaskProcessor.java:50) ~[classes/:na]
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) [na:1.8.0_31]
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) [na:1.8.0_31]
    at java.lang.Thread.run(Thread.java:745) [na:1.8.0_31]
01:26:36.603 [Chunk-Updater-6] ERROR o.t.u.concurrency.TaskProcessor - Error in thread Chunk-Updater-6
java.lang.NullPointerException: null
...

Seemed to happen again even after resetting saves + config. Reverted to plain develop and it went away.

As for @flo's earlier comment about updating the preview for every change I think that's how it worked at one point, but it would stall the UI while regenerating which got annoying if you wanted to change multiple settings, thus the apply button :-) Unsure if that would be an issue still. +1 for a "Random!" button shuffling seed + all config :D

msteiger · 2015-05-19T18:34:05Z

I could replicate the error @flo reported, but it's not related to this PR. It doesn't reload the asset properly, since it doesn't get notified on parameter changes. We'll need a notification system first. I'll just deactivate it until this is in place.

I'll also prepare a few PRs in the affected modules, so we can merge them at the same time.

I cannot imagine how this PR could affect chunk generation and sunlight computation in particular. Does it happen after you start the game or in the preview screen?

Cervator · 2015-05-19T18:51:09Z

It happened on exiting the game from inside a height map world. Nothing special done or set up, just default everything, new game in heightmap, exit game, boom.

I don't imagine it relates to sunlight so much as some hidden issue leaving the game in an unstable state at the time of exit. I'll try it again tonight in case there was something wrong with my system last night.

If the tiny PR modules are a hassle you can just self-merge this then push the fixes directly to modules after :-)

Cervator · 2015-05-20T03:01:45Z

Okay, now I can't replicate the crash. Weird.

@msteiger please merge this at your convenience then tweak the affected modules :-)

flo · 2015-05-23T11:19:54Z

@msteiger about the Context: You can use the available Context instance everywhere were you use the CoreRegistry as the former is just forwarding the calls to the context.

About the deletion: I suggest we go for no deletion but create a issue for it. The issue doesn't need to be addressed in this pull request.

Conflicts: engine/src/main/java/org/terasology/engine/modes/loadProcesses/CreateWorldEntity.java engine/src/main/java/org/terasology/rendering/nui/layers/mainMenu/ConfigWorldGenScreen.java engine/src/main/java/org/terasology/rendering/nui/layers/mainMenu/PreviewWorldScreen.java engine/src/main/java/org/terasology/world/generator/plugin/TempWorldGeneratorPluginLibrary.java

GooeyHub · 2015-05-24T07:42:25Z

Refer to this link for build results (access rights to CI server needed):
http://jenkins.terasology.org/job/TerasologyPRs/98/
Hooray Jenkins reported success with all tests good!

GooeyHub · 2015-05-24T08:12:12Z

Refer to this link for build results (access rights to CI server needed):
http://jenkins.terasology.org/job/TerasologyPRs/100/
Hooray Jenkins reported success with all tests good!

msteiger · 2015-05-24T08:14:04Z

Afaict, all issues are resolved now. I'm merging this now so we can move on with follow-up changes without getting straight into merge conflict hell.

Merge world config screen into preview screen

msteiger · 2015-05-25T08:11:05Z

All affected modules have been updated.

Cervator added the in progress label May 14, 2015

flo reviewed May 16, 2015
View reviewed changes

msteiger added 8 commits May 18, 2015 17:35

Support multi-threaded preview generation

8e0b9b8

Add support for lambda in WaitPopup

900125d

Merge config screen into preview screen

8b961dc

Fix TtA world preview

1ae14d7

Convert "Apply" button to "Recompute" in PreviewWorldGen

e71090a

Support changing seed values in world generators

7562c7a

Replace Optional world configurator with stub

72e7355

Trigger update of preview screen on open

6f4020c

msteiger force-pushed the world_preview branch from addd45d to 6f4020c Compare May 18, 2015 15:51

flo mentioned this pull request May 23, 2015

Remove CoreRegistry usage: Part III + Improve ComponentSystemManager life cycle #1739

Merged

msteiger added 2 commits May 23, 2015 18:45

Update AbstractBaseWorldGenerator for changes world gen interfaces

b6a7ff0

msteiger force-pushed the world_preview branch from 19c859b to f9957aa Compare May 24, 2015 07:32

Fix merging issues

ebaa56a

msteiger added a commit that referenced this pull request May 24, 2015

Merge pull request #1726 from msteiger/world_preview

b2bcc42

Merge world config screen into preview screen

msteiger merged commit b2bcc42 into MovingBlocks:develop May 24, 2015

Cervator removed the in progress label May 24, 2015

Cervator added this to the !NEXT: v0.54.0 milestone May 25, 2015

Cervator mentioned this pull request May 30, 2015

Use new asset library that will be introduced by pull request #1740 Terasology-Archived/Pathfinding#12

Merged

msteiger deleted the world_preview branch June 25, 2015 18:29

Cervator mentioned this pull request Oct 8, 2016

Crash on "preview" or "config" buttons on create world screen, TTA active #1524

Closed

jdrueckert mentioned this pull request Oct 18, 2023

refactor: merge universe setup into world pregen screen #5122

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge world config screen into preview screen #1726

Merge world config screen into preview screen #1726

msteiger commented May 14, 2015

Cervator commented May 15, 2015

flo May 16, 2015

flo commented May 16, 2015

msteiger commented May 17, 2015

Cervator commented May 17, 2015

GooeyHub commented May 18, 2015

GooeyHub commented May 18, 2015

msteiger commented May 18, 2015

Cervator commented May 19, 2015

msteiger commented May 19, 2015

Cervator commented May 19, 2015

Cervator commented May 20, 2015

flo commented May 23, 2015

GooeyHub commented May 24, 2015

GooeyHub commented May 24, 2015

msteiger commented May 24, 2015

msteiger commented May 25, 2015

Merge world config screen into preview screen #1726

Merge world config screen into preview screen #1726

Conversation

msteiger commented May 14, 2015

Cervator commented May 15, 2015

flo May 16, 2015

Choose a reason for hiding this comment

flo commented May 16, 2015

msteiger commented May 17, 2015

Cervator commented May 17, 2015

GooeyHub commented May 18, 2015

GooeyHub commented May 18, 2015

msteiger commented May 18, 2015

Cervator commented May 19, 2015

msteiger commented May 19, 2015

Cervator commented May 19, 2015

Cervator commented May 20, 2015

flo commented May 23, 2015

GooeyHub commented May 24, 2015

GooeyHub commented May 24, 2015

msteiger commented May 24, 2015

msteiger commented May 25, 2015