OrderOfThePorcupine / ProjectPorcupine

Project Porcupine: A Base-Building Game...in Space!
GNU General Public License v3.0
299 stars 89 forks source link

[WIP] DialogBoxUI Changes #262

Open BraedonWooding opened 5 years ago

BraedonWooding commented 5 years ago

There is not much left to do other than cleanup and conversion of the rest of the dialog boxes.

Slightly better diff values

Because this PR removes so many prefabs it's diff values are wayyy too high and just too annoying to understand so this is just a chart using the following script

Language Lines Added Lines Removed Net
JSON 145 44 +101
XML 0 33 -33
MetaFiles 121 570 -449
C# 2339 2534 -195
Prefabs 1385 -18778 -17,393
Lua 7 -30 -23
Unity +253 -251 +2

What does this PR intend to do

What does this PR intend not to do

List of changes

Todo

Example of changes

private IEnumerator CheckIfSaveGameBefore(string prompt)
{
    bool saveGame = false;
    cancel = false;

    dialogManager.dialogBoxPromptOrInfo.SetPrompt(prompt);
    dialogManager.dialogBoxPromptOrInfo.SetButtons(DialogBoxResult.Yes, DialogBoxResult.No, DialogBoxResult.Cancel);

    dialogManager.dialogBoxPromptOrInfo.Closed = () =>
    {
        if (dialogManager.dialogBoxPromptOrInfo.Result == DialogBoxResult.Yes)
        {
            saveGame = true;
        }

        if (dialogManager.dialogBoxPromptOrInfo.Result == DialogBoxResult.Cancel)
        {
            cancel = true;
        }
    };

    dialogManager.dialogBoxPromptOrInfo.ShowDialog();

    while (dialogManager.dialogBoxPromptOrInfo.gameObject.activeSelf)
    {
        yield return null;
    }

    if (saveGame)
    {
        dialogManager.dialogBoxSaveGame.ShowDialog();
    }
}

private IEnumerator OnButtonNewWorldCoroutine()
{
    StartCoroutine(CheckIfSaveGameBefore("prompt_save_before_creating_new_world"));

    while (dialogManager.dialogBoxSaveGame.gameObject.activeSelf || dialogManager.dialogBoxPromptOrInfo.gameObject.activeSelf)
    {
        yield return null;
    }

    if (!cancel)
    {
        this.CloseDialog();
        dialogManager.dialogBoxPromptOrInfo.SetPrompt("message_creating_new_world");
        dialogManager.dialogBoxPromptOrInfo.ShowDialog();

        SceneController.ConfigureNewWorld();
    }
}

To

newWorld.onClick.AddListener(() => {
    var data = new Dictionary<string, object>()
    {
        { "Prompt", "prompt_save_before_creating_new_world" },
        { "Buttons", new string[] { "button_yes", "button_no", "cancel" } }
    };
    GameController.Instance.DialogBoxManager.ShowDialogBox("Prompt", data, (res) => {
        if (res["ExitButton"].ToString() == "button_yes")
        {
            // save game
            GameController.Instance.DialogBoxManager.ShowDialogBox("Save", null, (_) => {
                GameController.Instance.DialogBoxManager.SoftCloseTopDialog();
                GameController.Instance.DialogBoxManager.ShowDialogBox("LoadingScreen");
                SceneController.ConfigureNewWorld();
            });
        }
        else if (res["ExitButton"].ToString() == "button_no")
        {
            // dont save game
            // so just load
            GameController.Instance.DialogBoxManager.SoftCloseTopDialog();
            GameController.Instance.DialogBoxManager.ShowDialogBox("LoadingScreen");
            SceneController.ConfigureNewWorld();
        }
        // else we don't have to do anything
        // the cancel will just close the prompt save window
        // and since it won't then open the load window
        // it'll just go back to the options menu as normal
    });
});

Side note: why are callbacks better than IEnumerators here?

Images

Since someone almost always asks to see what the changes visually look like here they are;

screen shot 2019-03-03 at 11 15 16 pm

screen shot 2019-03-03 at 11 15 55 pm screen shot 2019-03-03 at 11 16 03 pm screen shot 2019-03-03 at 11 20 59 pm screen shot 2019-03-03 at 11 21 42 pm

koosemose commented 5 years ago

Since this is a pretty major change to our UI system, when you get to documentation stage, please include a full example of creating a dialog from start to finish that we can include in the wiki, beyond just code documentation.

BraedonWooding commented 5 years ago

I'll make sure to include it along with including a few examples of building the UI specifically (how to use the layout groups to make it so that some elements are at the top down and some go from the bottom up).

NogginBops commented 5 years ago

I'm not the biggest fan of using a dictionary for the UI data.

Right now the specific dictionary data is coupled with a very specific InitializeElement call. And all of the dialog boxes are their own classes so I feel that the dictionary data should just be broken down into constructor arguments (or args for the init) for the different dialog boxes. It's confusing (as there are a lot of steps, and not directly visible that the data is connected to a very specific class) and removes static type checking from the code. So I would like to see creation of each type of dialog box explicitly instead of doing some type of matching on input args.

That would also remove a lot of the reflection and runtime type inference and in general lead to cleaner code.

koosemose commented 5 years ago

First note: that red circle with the X looks horrible, I could've swore that at some point we had a much subtler and fitting X... but perhaps the fact that it's always on just makes it more obvious to me (also it's much bigger in yours than it is currently). It's also way too large (even more so in the job list). If possible I would prefer to have the "only show the delete button when selected" functionality we currently have.

The formatting in the load dialog looks much worse ( both the timestamp being the same font size and it being black rather than white text).

The spacing between the saves in the load is larger, and also it's lost the alternating color we currently have (which really helps in visually distinguishing different saves) which would really benefit the trade screen as well.

The quest screen could also use some kind of separation between each quest, in addition to the scrolling box having something to distinguish it from the rest of the dialog (rather a boarder or a subtly different background color).

The tradescreen could also use white text, and smaller buttons relative to the text on the buttons.

Some of these may be how it is currently (but that needs improved), or could be a limitation of your system, or possibly just how you've chosen to style them at this stage. But I can't easily tell between the second two, and if we're intending to entirely replace our UI with this system, we need for it to not limit us in our presentation.

BraedonWooding commented 5 years ago

@Koosemose, I’ve listed some of those things already :), I’m going to do a few more sweeps before it’s finished. There are no limitations honestly but there are things that are harder. Most of those things though are easy

BraedonWooding commented 5 years ago

@nogginbops, I think you are misunderstanding the point of the UI system. There is only meant to be one enter call..?

This is because it prioritises modding and flexibility, which means it can’t have a lot of static safety. This however is not a problem since with mods you already have to run the game to see if you have any compilation issues :). The same thing about static safety can be said about any of our json files as well. Also having the get be not a string makes it so you can’t have multiple of the same dialog on the stack (which some dialogs need)

I personally don’t see how having unique constructors for dialog boxes will clean up code at all that just reverts it to the prior ugliness of the previous system. And it inhibits modding.

I also don’t see any steps for calling a dialog box?

The OLD one you had to do the following in a specific order; GetDialogBox (either by name which faces the same problem we have now or hard code them - ew), then you had to do stuff like .SetPrompt() and so on. Then show.

Now u just have a single call to show a box and you setup keypairs of data beforehand. There is nothing confusing personally about that? Also have a look at the example i put up, i think that shows how much cleaner my system is for complex dialogs (not only is it shorter it avoids IEnumerators yay)

BraedonWooding commented 5 years ago

Also, another benefit is that if someone wants to override joblist to give it a new UI they can either override the class and delete parts of the base call to the initialise (something they couldn’t do with constructors), or they could just start from scratch.

Allowing them to make a mod like “BetterJobs” (rimworld example) where jobs are broken down more with more priorities :).

They are of course still encouraged to follow the psuedo API using the same options but they can also add new ones for their code.

NogginBops commented 5 years ago

@BraedonWooding It's not hard just having one entry point with static typing. You would just do something like:

DialogBox dialog = new YesNoPrompt(Title: "Do you really want to exit?", Yes: () => {}, No: () => {});
GameController.Instance.DialogBoxManager.ShowDialogBox(dialog);

I'm using named arguments so it's more understandable.

Here you have the same (almost, described below) moddability and flexibility, but with 100% more static type-checking. It also forces you to actually supply the data, and the code that creates the dialog doesn't need to do any "does this data actually exist" checks.

And for the "better jobs" mod it only really needs to change what dialog box is opened when you click the job button or just make a virtual function that a mod can override that actually opens the dialog. The latter is probably preferable. Something like: delegate DialogBox DialogCreator(...); that the mod can then change the function reference called.

public DialogCreator CreateExitPrompt;
...
DialogBox dialog = CreateExitPrompt();
GameController.Instance.DialogBoxManager.ShowDialogBox(dialog);

The function reference might even be described in json so it's super easy to change.

This way we don't have to sacrifice type safety. And that is a really good thing.

BraedonWooding commented 5 years ago

I have updated the UI but I haven't updated the images here yet. I'll get to it sometime in the next week. I'm pretty bogged down so I only get 30 mins or so every day (often none).

Atleast another week - two weeks before completion.

BraedonWooding commented 5 years ago

Kinda screwed up the git history oops, that's what I get for doing a pull from upstream and not instantly merging to conflicts and sorting that manually. I'll fix it up soon ish, just no point spending the time right now :).