Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: merge world config in setup screen #5226

Merged
merged 19 commits into from
Apr 21, 2024

Conversation

jdrueckert
Copy link
Member

@jdrueckert jdrueckert commented Mar 17, 2024

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

@jdrueckert jdrueckert added Topic: UI/UX Requests, Issues and Changes related to screens, artwork, sound and overall user experience Size: M Medium-sized effort likely requiring some research and work in multiple areas Status: Needs Testing Requires to be tested in-game for reproducibility Type: Refactoring Request for or implementation of pure and automatic refactorings, e.g. renaming, to improve clarity labels Mar 17, 2024
@jdrueckert jdrueckert added this to the 2023 Revive - Milestone 3 milestone Mar 17, 2024
@jdrueckert jdrueckert self-assigned this Mar 17, 2024
…com:MovingBlocks/Terasology into refactor/merge-world-config-in-setup-screen
* this allows for property-specific updates
* interested parties can subscribe to receive change notifications for all or specific properties available
* change notifications include information about the changed property as well as the old and new values
* no longer interested parties can subscribe from all or specific properties
* subscriptions to all properties will remain intact in case a no longer interested party cancels their subscription for specific properties
@jdrueckert jdrueckert changed the base branch from develop to feat/subscribable-world-configurator April 7, 2024 19:13
Base automatically changed from feat/subscribable-world-configurator to develop April 14, 2024 17:46
@jdrueckert jdrueckert marked this pull request as ready for review April 14, 2024 17:46
@BenjaminAmos BenjaminAmos self-requested a review April 14, 2024 19:03
Copy link
Contributor

@BenjaminAmos BenjaminAmos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +291 to +292
getManager().pushScreen(MessagePopup.ASSET_URI, MessagePopup.class).setMessage("No world generators registered!",
"Please select at least one module that provides a world generator!");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes good point, I'll add that 👍

@@ -299,11 +355,14 @@ private void addNewWorld(WorldGeneratorInfo worldGeneratorInfo) {
WorldGenerator worldGenerator = WorldGeneratorManager.createWorldGenerator(worldGeneratorInfo.getUri(), context, environment);
worldGenerator.setWorldSeed(universeWrapper.getSeed());
universeWrapper.setWorldGenerator(worldGenerator);
context.put(UniverseWrapper.class, universeWrapper);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@@ -299,11 +355,14 @@ private void addNewWorld(WorldGeneratorInfo worldGeneratorInfo) {
WorldGenerator worldGenerator = WorldGeneratorManager.createWorldGenerator(worldGeneratorInfo.getUri(), context, environment);
worldGenerator.setWorldSeed(universeWrapper.getSeed());
universeWrapper.setWorldGenerator(worldGenerator);
context.put(UniverseWrapper.class, universeWrapper);
} catch (UnresolvedWorldGeneratorException e) {
//TODO: this will likely fail at game creation time later-on due to lack of world generator - don't just ignore this
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

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.

Copy link
Contributor

@BenjaminAmos BenjaminAmos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jdrueckert jdrueckert merged commit 84dd22e into develop Apr 21, 2024
10 checks passed
@jdrueckert jdrueckert deleted the refactor/merge-world-config-in-setup-screen branch April 21, 2024 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Size: M Medium-sized effort likely requiring some research and work in multiple areas Status: Needs Testing Requires to be tested in-game for reproducibility 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
Projects
Status: No status
Status: No status
Development

Successfully merging this pull request may close these issues.

AGS World Generator Configuration does not fail gracefully
2 participants