refactor: merge universe setup into world pregen screen #5122

jdrueckert · 2023-07-17T20:57:45Z

Contains

Merging UniverseSetupScreen and WorldPreGenerationScreen incl.

merging .ui representations
merging .java classes
using UniverseWrapper for persisting state
binding UI widgets to persisted state

How to test

Start Terasology
Create World via Advanced Game Setup
Play around with world setup screen elements

Outstanding before merging

Based on #5118.

merge UI elements
merge widget bindings and screen logic
use UniverseWrapper instead of local copies of target world and seed
use universe seed and properly randomize seed on re-roll
fix initial world generator dropdown binding
remove worlds element in lower right screen section
update preview on changing seed field content
memorize modified (manually or via re-roll) seed field content (currently reset always to initial seed)

Follow-Ups

merge WorldSetupScreen into UniverseSetupScreen
- basically doing Merge world config screen into preview screen #1726 again
remove CoreRegistry in favor of Context
harden against preview aborts (currently lead to weird state)

…setEnvironment

…w gen - world generator can no longer be null if target world is not null - preview is generated during screen initialization due to binding

jdrueckert · 2023-07-20T20:11:51Z

@skaldarnar with bb7fab6 I run into the OOM issue again during preview generation:

java.util.concurrent.ExecutionException: java.lang.OutOfMemoryError: Java heap space
	at java.base/java.util.concurrent.FutureTask.report(FutureTask.java:122)
	at java.base/java.util.concurrent.FutureTask.get(FutureTask.java:191)
	at org.terasology.engine.rendering.nui.layers.mainMenu.preview.FacetLayerPreview.render(FacetLayerPreview.java:130)
	at org.terasology.engine.rendering.nui.layers.mainMenu.UniverseSetupScreen.lambda$updatePreview$7(UniverseSetupScreen.java:416)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.lang.Thread.run(Thread.java:829)
Caused by: java.lang.OutOfMemoryError: Java heap space

Without it I don't, but I don't get how the changes in that commit would result in running into this kind of a loop there 🤔 any ideas?

skaldarnar · 2023-07-20T20:28:56Z

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

@@ -162,7 +159,7 @@ public WorldGeneratorInfo get() {
                @Override
                public void set(WorldGeneratorInfo value) {
                    if (value != null) {
-                        if (universeWrapper.getTargetWorld() == null || !value.getUri().equals(universeWrapper.getTargetWorld().getWorldGenerator().getUri())) {
+                        if (universeWrapper.getWorldConfigurator() == null || !value.getUri().equals(universeWrapper.getWorldGenerator().getUri())) {


why configurator here?

jdrueckert · 2023-10-15T12:22:43Z

...gy/engine/rendering/nui/layers/mainMenu/advancedGameSetupScreen/AdvancedGameSetupScreen.java

-    public void setUniverseWrapper(UniverseWrapper wrapper) {
-        universeWrapper = wrapper;
+    public void setEnvironment(UniverseWrapper wrapper) {
+        CoreRegistry.put(UniverseWrapper.class, wrapper);


Theoretically, the idea was to do the following:

context.put(UniverseWrapper.class, wrapper); CoreRegistry.setContext(context);

However, this does not work and leads to an NPE in UniverseSetupScreen.java:182 when attempting to access UniverseWrapper from a context that it's not in. At this moment, it's unclear, why this does not work.

Do you want to persist this information in the code as comment instead of just here on this PR?

- especially relevant for coming back from WorldSetupScreen

…-screen

…ld-pregen-screen' into refactor/merge-universe-into-world-pregen-screen

…-screen

…ld-pregen-screen' into refactor/merge-universe-into-world-pregen-screen

…-screen

jdrueckert · 2023-10-18T21:56:16Z

@skaldarnar from my side this would be ready for review

…-screen

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

skaldarnar · 2023-10-27T21:21:19Z

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

-    public void setWorld(Context subContext, WorldSetupWrapper worldSelected)
-            throws UnresolvedWorldGeneratorException {
-        world = worldSelected;
+    public void setWorld(Context subContext, UniverseWrapper universe) {


I'm annoyed by these HiddenFieldChecks - I'd like to have an overview in how many places this would actually cause trouble right now, and if we find none, disable this check.

I had it run on a iota workspace and it came back with more than 2k findings for hidden fields.
I think most of them are constructors or setters, but it's hard to filter those out.

skaldarnar · 2023-10-27T21:22:00Z

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

-        } else {
-            worldGenerator = world.getWorldGenerator();
-        }
+        worldGenerator = universe.getWorldGenerator();


The world generator in the universe can no longer be null?

world generator is set at initialize time (see UniverseSetupScreen#168)
I'd say it not being set is sufficiently unlikely that we don't need the null check anymore.
if you disagree I can re-add it of course

skaldarnar · 2023-10-27T21:23:37Z

...gy/engine/rendering/nui/layers/mainMenu/advancedGameSetupScreen/AdvancedGameSetupScreen.java

+        super.onScreenOpened();
+
+        final UIText seed = find("seed", UIText.class);
+        if (seed != null) {


Hm, if we need to check for null here, should we at least log a warning or something in case the seed text widget could not be found? I mean, it should be there when the screen is opened, right?

tbh I'm not sure, why we need the null check(s) (there's one in AdvancedGameSetupScreen::initialise, too)... I would agree that the text widget should be there when the screen is opened and it should even already be there when it is initialized...
I don't see a valid reason why it should ever not be, so I'll remove the null checks.

I think the main reason is that we don't have automated tests that spot NPEs like this. For instance, they can appear if we rename the UI control in the some .ui file but not here, or vice versa, move UI controls around but forget about the code, etc.

It will be pretty apparent while testing locally, but we don't have automated tests that "click through all the screens" that would catch that...

...gy/engine/rendering/nui/layers/mainMenu/advancedGameSetupScreen/AdvancedGameSetupScreen.java

skaldarnar · 2023-10-27T21:29:11Z

...gy/engine/rendering/nui/layers/mainMenu/advancedGameSetupScreen/AdvancedGameSetupScreen.java

-    public void setUniverseWrapper(UniverseWrapper wrapper) {
-        universeWrapper = wrapper;
+    public void setEnvironment(UniverseWrapper wrapper) {
+        CoreRegistry.put(UniverseWrapper.class, wrapper);


Do you want to persist this information in the code as comment instead of just here on this PR?

skaldarnar · 2023-10-27T21:31:24Z

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


        DependencyResolver resolver = new DependencyResolver(moduleManager.getRegistry());
        ResolutionResult result = resolver.resolve(config.getDefaultModSelection().listModules());
        if (result.isSuccess()) {
            environment = moduleManager.loadEnvironment(result.getModules(), false);
+            //BlockFamilyLibrary library =  new BlockFamilyLibrary(environment, context);


Why is this commented out?

This was originally part of initAssets but library was never used afterwards.
IIRC I commented it out to see if it breaks anything and wanted to check on possible side-effects it might've been needed for. During my manual tests I didn't see any indicators that commenting it out broke anything.
I can either remove it entirely or replace it with a comment stating that this used to be done in case we see weird behavior in the future that might be related... Which would you prefer?

…-screen

jdrueckert changed the title ~~Refactor/merge universe into world pregen screen~~ refactor: merge universe setup into world pregen screen Jul 17, 2023

jdrueckert assigned jdrueckert and skaldarnar Jul 17, 2023

jdrueckert added Topic: UI/UX Requests, Issues and Changes related to screens, artwork, sound and overall user experience Type: Refactoring Request for or implementation of pure and automatic refactorings, e.g. renaming, to improve clarity labels Jul 17, 2023

jdrueckert added this to the 2023 Revive - Milestone 1 milestone Jul 17, 2023

jdrueckert and others added 10 commits July 18, 2023 18:25

refactor: merge StartPlaying screen into WorldPreGeneration screen

fcf8723

remove unused code

b02c473

ui: merge world pregen screen elements into universe setup screen

dc0b5a5

refactor: move code from WorldPreGenerationScreen to UniverseSetupScreen

c96c4c6

ui: add seed field

55ab06b

refactor: reuse universe seed for world, properly randomize on re-roll

a883a24

refactor: create new WorldSetupWrapper on world generator selection

3f52956

refactor: get/set seed always from/in universe wrapper

df3f0f9

refactor: get/set selected world from/in universe wrapper

afc1eb8

chore: only do something if different world generator selected

06217b5

skaldarnar force-pushed the refactor/merge-universe-into-world-pregen-screen branch from 22fe085 to 06217b5 Compare July 18, 2023 16:45

skaldarnar changed the base branch from develop to refactor/merge-advanced-world-setup-screens July 18, 2023 16:46

skaldarnar and others added 7 commits July 20, 2023 19:37

set world info initially and remove unncessary preview gen code from …

0f2082b

…setEnvironment

chore: remove unused imports

55073dc

refactor: always set world generator when setting target world

89e1f8f

chore: remove no longer used worldpregen screen and ui

fbc0757

chore: remove unnecessary world generator check and duplicated previe…

17d911e

…w gen - world generator can no longer be null if target world is not null - preview is generated during screen initialization due to binding

refactor: remove worldGeneratorInfo from worldSetupWrapper

0a26c4d

refactor: merge WorldSetupWrapper into UniverseWrapper

bb7fab6

skaldarnar reviewed Jul 20, 2023

View reviewed changes

fix: auto-completion typo

2102121

Base automatically changed from refactor/merge-advanced-world-setup-screens to develop July 20, 2023 21:05

refactor: remove custom title from WorldInfo

4b15e6d

jdrueckert commented Oct 15, 2023

View reviewed changes

jdrueckert added 11 commits October 15, 2023 21:42

refactor: return empty string if seed is null

3632055

feat: save modified seed when going back to AGS screen

a0ccb20

feat: update preview on manual seed change

194f87d

feat: update preview on opening screen

617dd1f

- especially relevant for coming back from WorldSetupScreen

Merge branch 'develop' into refactor/merge-universe-into-world-pregen…

0701a8e

…-screen

checkstyle: remove unused imports

3cd4817

Merge remote-tracking branch 'origin/refactor/merge-universe-into-wor…

83d0a25

…ld-pregen-screen' into refactor/merge-universe-into-world-pregen-screen

checkstyle: fix long line finding

8c870ec

Merge branch 'develop' into refactor/merge-universe-into-world-pregen…

656098c

…-screen

refactor: minor cleanups

a0b7f35

Merge remote-tracking branch 'origin/refactor/merge-universe-into-wor…

ff41975

…ld-pregen-screen' into refactor/merge-universe-into-world-pregen-screen

jdrueckert requested a review from skaldarnar October 16, 2023 22:15

jdrueckert marked this pull request as ready for review October 16, 2023 22:16

Merge branch 'develop' into refactor/merge-universe-into-world-pregen…

2386627

…-screen

jdrueckert added 5 commits October 22, 2023 15:43

qa: remove unnecessary local before return

c3a0bd2

Merge branch 'develop' into refactor/merge-universe-into-world-pregen…

e5f023c

…-screen

qa: if statement nesting not necessary

5c2cb32

qa: remove unused local variable

f1431ca

qa: remove unused local variable

6ae1c68

skaldarnar previously approved these changes Oct 27, 2023

View reviewed changes

refactor: address review comments

cd21b6f

jdrueckert dismissed skaldarnar’s stale review via cd21b6f October 30, 2023 23:13

Merge branch 'develop' into refactor/merge-universe-into-world-pregen…

d48ae6d

…-screen

skaldarnar approved these changes Nov 12, 2023

View reviewed changes

Merge branch 'develop' into refactor/merge-universe-into-world-pregen…

3ff1e7c

…-screen

jdrueckert merged commit c237660 into develop Nov 13, 2023
8 checks passed

jdrueckert deleted the refactor/merge-universe-into-world-pregen-screen branch November 13, 2023 12:05

jdrueckert mentioned this pull request Mar 17, 2024

refactor: merge world config in setup screen #5226

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: merge universe setup into world pregen screen #5122

refactor: merge universe setup into world pregen screen #5122

jdrueckert commented Jul 17, 2023 •

edited

Loading

jdrueckert commented Jul 20, 2023

skaldarnar Jul 20, 2023

jdrueckert Oct 15, 2023

skaldarnar Oct 27, 2023

jdrueckert Oct 30, 2023

jdrueckert commented Oct 18, 2023

skaldarnar Oct 27, 2023

jdrueckert Oct 29, 2023

skaldarnar Oct 27, 2023

jdrueckert Oct 30, 2023

skaldarnar Oct 27, 2023

jdrueckert Oct 30, 2023

skaldarnar Nov 12, 2023

skaldarnar Oct 27, 2023

skaldarnar Oct 27, 2023

jdrueckert Oct 29, 2023

refactor: merge universe setup into world pregen screen #5122

refactor: merge universe setup into world pregen screen #5122

Conversation

jdrueckert commented Jul 17, 2023 • edited Loading

Contains

How to test

Outstanding before merging

Follow-Ups

jdrueckert commented Jul 20, 2023

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 commented Oct 18, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdrueckert commented Jul 17, 2023 •

edited

Loading