refactor: merge world config in setup screen

[jdrueckert]

Contains

Continues Advanced Game Setup (AGS) overhaul started in #5118 and #5122.
Merges WorldSetupScreen into UniverseSetupScreen.

Fixes #4810 as a side effect.

How to test

1. Start Terasology
2. Start creating a world
3. Go to Advanced Game Setup (AGS)
4. Continue to Universe Setup
5. Play around with the screen, check for unclear UI elements, unexpected UI behavior, etc.
6. Verify that the game can be started

Outstanding before merging

• Trigger a preview update when configuration is changed
• clean up code where necessary
• improve documentation where necessary

[BenjaminAmos]

This is an intial review from a brief look at the changes here. I have not thoroughly tested this in-game yet.

[BenjaminAmos]

Should the user be sent back to the module selection screen when this pop-up is dismissed?

[jdrueckert]

yes good point, I'll add that 👍

[BenjaminAmos]

You should not place items in the context in random classes. Ideally everything would be registered in clearly-defined places, such as using special class annotations (like @Share) or at program start-up.

Would a class-level variable suffice here?

[jdrueckert]

The problem I was facing here, is that using a class-level injected variable for the universeWrapper doesn't work.
Once the user entered the UniverseSetupScreen, a world generator and seed are selected and we want to keep them, even if the user is going back and forth between the module selection and this screen. IIRC when I worked on the first refactoring stages, I noticed, that updating the universeWrapper with world generator and seed didn't persist correctly, that's why I did it this way now.

I feel like there's still some weirdness between context, core registry and DI.
We do have a proposed task in https://github.com/MovingBlocks/Terasology/milestone/53 to work on removing core registry usages in favor of context which would include figuring some of these issues out, but we didn't get to it yet.

[BenjaminAmos]

It seems that this class does a lot of interference with the context, so this is inline with what was there before. As much as I would prefer a cleaner architecture, it is managable so long as it stays in the main menu context (each new/loaded game should have its own fresh context anyway).

[BenjaminAmos]

Could we at least show a warning pop-up to the player in this case?

[jdrueckert]

Looking at createWorldGenerator():

    public static WorldGenerator createWorldGenerator(SimpleUri uri, Context context, ModuleEnvironment environment)
            throws UnresolvedWorldGeneratorException {
        for (Class<?> generatorClass : environment.getTypesAnnotatedWith(RegisterWorldGenerator.class)) {
            RegisterWorldGenerator annotation = generatorClass.getAnnotation(RegisterWorldGenerator.class);
            Name moduleName = environment.getModuleProviding(generatorClass);
            if (moduleName == null) {
                throw new UnresolvedWorldGeneratorException("Cannot find module for world generator " + generatorClass);
            }
            SimpleUri generatorUri = new SimpleUri(moduleName, annotation.id());
            if (generatorUri.equals(uri)) {
                WorldGenerator worldGenerator = loadGenerator(generatorClass, generatorUri);
                InjectionHelper.inject(worldGenerator, context);
                return worldGenerator;
            }
        }
        throw new UnresolvedWorldGeneratorException("Unable to resolve world generator '" + uri + "' - not found");
    }

I'm thinking, we want to show a warning pop-up to the player if the selected world generator cannot be resolved
throw new UnresolvedWorldGeneratorException("Unable to resolve world generator '" + uri + "' - not found");
but not if for any of the available world generators an associated module cannot be found
throw new UnresolvedWorldGeneratorException("Cannot find module for world generator " + generatorClass);

I'm not a fan of string-comparing error messages, but maybe a we can introduce another Exception type to differentiate the two cases? What do you think?

[BenjaminAmos]

I think a missing module deserves its own exception, as that is a different issue. UnresolvedDependencyException may be appropriate but defining a new exception would be fine too.

[BenjaminAmos]

There are a few things I noticed which might still need to be resolved after merging this:

• The "Universe Set-up" screen still shows "World Pre-generation" as its title.

• The text:

  Pick a world generator.
  World Generators:

  seems a bit redundant. Maybe just keep the World Generators: part?

• The Re-Roll button is not the same height as the seed text field.

• Headings between each world configuration option should be more clearly delineated (for example, having a box around each expandable section). This is likely a general issue with PropertyLayout however.

[BenjaminAmos]

There are a few things that could be improved (as mentioned before) but nothing that should block this.