New create game flow backend | Part 2 #3383

TheFlash98 · 2018-06-05T21:09:09Z

Contains

The backend of the new create game flow. The player can now select different world generators and add a number of worlds to the game. Each world has a unique seed and the seed you see on the AdvanceGameSetupScreen is for the Universe. Each world is previewable individually on the WorldPreGenerationScreen. You have the option to configure and change the properties of your world in both UniverseSetupScreen and WorldPreGenerationScreen. There is a configure button which leads you to the WorldSetupScreen where properties can be edited and the changes are successfully previewed in the WorldPreGenerationScreen.

Outstanding before merging

Making a world with Height Map causes a crash.
Game title (?)

A video of what's done: https://www.youtube.com/watch?v=qhKGQjEONmA

GooeyHub · 2018-06-05T21:21:50Z

Hooray Jenkins reported success with all tests good!

GooeyHub · 2018-06-07T10:49:43Z

Hooray Jenkins reported success with all tests good!

GooeyHub · 2018-06-07T16:46:30Z

Hooray Jenkins reported success with all tests good!

GooeyHub · 2018-06-07T18:38:59Z

Hooray Jenkins reported success with all tests good!

Cervator

Well done. I'm having trouble finding anything not trivial :-)

Cervator · 2018-06-07T18:05:24Z

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

@@ -90,6 +90,7 @@
    private Config config;

    private boolean loadingAsServer;
+    private UniverseWrapper universeWrapper;


This might be a good candidate for some quick field javadoc

Cervator · 2018-06-07T18:12:37Z

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

+            }
+
+            SimpleUri uri = world.getWorldGeneratorInfo().getUri();
+            float timeOffset = 0.25f + 0.025f;


Magic numbers? Might be an idea to extract info a constant that could also explain what the number means

Cervator · 2018-06-07T18:15:10Z

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

+     * in which the player is about to spawn.
+     * @param targetWorld The world in which the player is going to spawn.
+     * @param targetWorldTexture The world texture generated in {@link WorldPreGenerationScreen} to be displayed on this
+     *                           screen.


You can probably just throw this one word back on the main line to keep it a one liner. I sometimes finding myself rewriting javadoc slightly to make more one liners for how nicely it lines up stuff :-)

Cervator · 2018-06-07T18:17:49Z

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

 public class UniverseSetupScreen extends CoreScreenLayer {

    public static final ResourceUrn ASSET_URI = new ResourceUrn("engine:universeSetupScreen");
+    List<WorldSetupWrapper> worlds = Lists.newArrayList();


Is this the intended access modifier for these two vars, or did you forgot to add in public or private ?

Cervator · 2018-06-07T18:20:45Z

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

@@ -188,10 +243,118 @@ private void recursivelyAddModuleDependencies(Set<Name> modules, Name moduleName
        }
    }

+    /**
+     * Called whenever the user decides to create a new world.


"create" -> "add" ? Might be a subtle difference or a subjective call. Adding a new entry to a list vs creating a new entry to a list.

Cervator · 2018-06-07T18:25:30Z

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

+ */
+package org.terasology.rendering.nui.layers.mainMenu;
+
+public class UniverseWrapper {


This one could definitely use some quick class level javadoc, although I do think the concept is readily understandable :-)

Cervator · 2018-06-07T18:28:12Z

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

+    private int seedNumber = 0;
+
+
+    public WorldPreGenerationScreen() {


Do you actually need this constructor? Utility classes usually have a private default constructor to disallow instantiation, but this seems a normal class and you get that constructor for free

Cervator · 2018-06-07T18:31:09Z

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

+     * Generates a texture and set's to the image view, thus previewing the world.
+     */
+    private void genTexture() {
+        int imgWidth = 384;


Should the size be variable in case the user resizes the window? If not maybe extract to a constant or two.

Resizing support could be added later, not important right now

set's -> sets
..sets it to..

You really need to be careful with your apostrophes.

Cervator · 2018-06-07T18:32:52Z

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

+            Class<? extends Component> clazz = params.get(key).getClass();
+            Component comp = config.getModuleConfig(worldGenerator.getUri(), key, clazz);
+            if (comp != null) {
+                worldConfig.setProperty(key, comp);       // use the data from the config instead of defaults


Generally I try to put comments above a line instead of later on the line. That might be personal preference and situational however.

Cervator · 2018-06-07T18:34:25Z

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

+        protected WorldConfigNumberBinding(WorldConfigurator config, String label, ComponentLibrary compLib, FieldMetadata<Object, ?> field) {
+            Class<?> type = field.getType();
+            if (type == Integer.TYPE || type == Integer.class) {
+                this.binding = new WorldSetupScreen.WorldConfigBinding<>(config, label, compLib,


You don't really need to include a line break here, on a single line this is 138 chars, not too bad, our style warning is at 175 IIRC. Again somewhat personal/situational

GooeyHub · 2018-06-07T19:03:03Z

Hooray Jenkins reported success with all tests good!

GooeyHub · 2018-06-07T19:44:31Z

Hooray Jenkins reported success with all tests good!

vampcat

Mostly minor style/jd changes.

vampcat · 2018-06-08T03:15:18Z

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

@@ -91,6 +91,11 @@

    private boolean loadingAsServer;

+    /**
+     * A UniverseWrapper object used here to determine if the game is single player or multi-player.


Consistency. Either use single-player or multi player, but not both - and specially not in the same line.

vampcat · 2018-06-08T03:22:13Z

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

    public static final ResourceUrn ASSET_URI = new ResourceUrn("engine:startPlayingScreen");
+    private Texture texture;


Again, let's try to maintain with the codebase at large. Almost everywhere we start with public static final variables at top, followed by public and private variables. So let's move ASSET_URI to the top.

Also, not a huge fan of "ASSET_URI" - it does not really tell me what the asset is. See if you can come up with a better name for it.

Every screen's urn in the game has been given the name "ASSET_URI". So I used the same on all my screens, for keeping it uniform. :-)

vampcat · 2018-06-08T03:35:48Z

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

+
+            SimpleUri uri = world.getWorldGeneratorInfo().getUri();
+            // This is multiplied by the number of seconds in a day (86400000) to determine the exact  millisecond at which the game will start.
+            float timeOffset = 0.50f;


Any reason to not make timeOffset final?

vampcat · 2018-06-08T03:37:04Z

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

 public class UniverseSetupScreen extends CoreScreenLayer {

    public static final ResourceUrn ASSET_URI = new ResourceUrn("engine:universeSetupScreen");
+    private List<WorldSetupWrapper> worlds = Lists.newArrayList();


Sigh, the "ASSET_URI" seems to be way too common in this part of the codebase.

vampcat · 2018-06-08T03:38:45Z

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

    @In
    private ModuleManager moduleManager;
-


Note that everywhere in TS code base (including the other class you modified in this PR) you leave a newline b/w two consecutive @in variables. Revert these deletions.

vampcat · 2018-06-08T03:55:47Z

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

+
+    /**
+     * Updates the preview according to any changes to made the configurator.
+     * Also pops up a message and keeps track of percentage world preview prepared.


..made to the configurator..

vampcat · 2018-06-08T03:56:27Z

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

+    /**
+     * This method takes the name of the selected world as String and return the corresponding
+     * WorldSetupWrapper object.
+     * @return {@link WorldSetupWrapper} object.


Not helpful.

vampcat · 2018-06-08T03:57:00Z

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

+    @In
+    private WorldGeneratorManager worldGeneratorManager;
+    @In
+    private Config config;


So two In variables have no newline, but one does? Wut?

vampcat · 2018-06-08T03:57:38Z

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

+    /**
+     * This method sets the world whose properties are to changed. This function is called before the screen comes
+     * to the forefront.
+     * @param subContext As there is a new environment created in the {@link UniverseSetupScreen}, it's passed to


See previous comment.

vampcat · 2018-06-08T03:58:30Z

engine/src/main/java/org/terasology/rendering/world/WorldSetupWrapper.java

+/**
+ * This class only has significance during the create game phase.
+ * Every world is a object of this class.
+ */


TheFlash98 · 2018-06-08T15:35:53Z

theflash98 & multi-world

TheFlash98 added 16 commits June 2, 2018 03:18

A few hits at switching Environments

80b9532

Add check for empty worlds list and popup a message

6f7fc64

Start of WorldSetup screen and new World class

656ca7b

Able to change property values for a world

a3747d7

Quick id corrections

5d74b47

Display world list in worldpregenscreen

aa6a4bd

Fix Scrollable of WorldSetupScreen

d86d2d7

Preview now loads up according to different configurations

8b8bb37

Link worldpren -> worldsetup

16ac9d5

Add zoom to preview screen

0311a53

re-roll the world seed

b436ae4

StartPlaying screen done

130e271

code clean up

ea8acf3

World -> WorldSetupWrapper

d210112

show preview when screen is opened

deedf14

Some java doc

5bf54e1

TheFlash98 changed the title ~~New create game~~ New create game flow backend | Part 2 Jun 5, 2018

Add universe wrapper for game name, seed and mulitplayer

11b1914

Give seed when game rum from NewGameScreen

03e9098

TheFlash98 added 2 commits June 7, 2018 23:56

Display world name on top of WorldSetup and StartPlaying screens

ccfe308

Change timeoffset to start game at 12 noon

3ca6d32

Cervator approved these changes Jun 7, 2018

View reviewed changes

TheFlash98 added 2 commits June 8, 2018 00:38

Change preview everytime zoom changes

f97587a

A few fixes as asked in the review

8830a3d

Cervator merged commit 8830a3d into MovingBlocks:create-game-v2 Jun 7, 2018

Cervator added this to the v3.0.0 milestone Jun 7, 2018

vampcat reviewed Jun 8, 2018

View reviewed changes

TheFlash98 mentioned this pull request Jun 8, 2018

New create game flow backend | Final steps #3385

Merged

2 tasks

Cervator mentioned this pull request Jun 18, 2018

GSOC 2018: First batch of UI changes for multi-world #3399

Merged

Cervator modified the milestones: v3.0.0, v2.1.0 Jun 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New create game flow backend | Part 2 #3383

New create game flow backend | Part 2 #3383

TheFlash98 commented Jun 5, 2018 •

edited

Loading

GooeyHub commented Jun 5, 2018

GooeyHub commented Jun 7, 2018

GooeyHub commented Jun 7, 2018

GooeyHub commented Jun 7, 2018

Cervator left a comment

Cervator Jun 7, 2018

Cervator Jun 7, 2018

Cervator Jun 7, 2018

Cervator Jun 7, 2018

Cervator Jun 7, 2018

Cervator Jun 7, 2018

Cervator Jun 7, 2018

Cervator Jun 7, 2018

vampcat Jun 8, 2018

Cervator Jun 7, 2018

Cervator Jun 7, 2018

GooeyHub commented Jun 7, 2018

GooeyHub commented Jun 7, 2018

vampcat left a comment

vampcat Jun 8, 2018

vampcat Jun 8, 2018

TheFlash98 Jun 8, 2018

vampcat Jun 8, 2018

vampcat Jun 8, 2018

vampcat Jun 8, 2018

vampcat Jun 8, 2018

vampcat Jun 8, 2018

vampcat Jun 8, 2018

vampcat Jun 8, 2018

vampcat Jun 8, 2018

TheFlash98 commented Jun 8, 2018

		private int seedNumber = 0;


		public WorldPreGenerationScreen() {

		public static final ResourceUrn ASSET_URI = new ResourceUrn("engine:startPlayingScreen");
		private Texture texture;

New create game flow backend | Part 2 #3383

New create game flow backend | Part 2 #3383

Conversation

TheFlash98 commented Jun 5, 2018 • edited Loading

Contains

Outstanding before merging

GooeyHub commented Jun 5, 2018

GooeyHub commented Jun 7, 2018

GooeyHub commented Jun 7, 2018

GooeyHub commented Jun 7, 2018

Cervator 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

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

GooeyHub commented Jun 7, 2018

GooeyHub commented Jun 7, 2018

vampcat 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

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

TheFlash98 commented Jun 8, 2018

TheFlash98 commented Jun 5, 2018 •

edited

Loading