[WIP] DialogBoxUI Changes #262
Conversation
|
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. |
|
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). |
|
|
||
| public override GameObject InitializeElement() | ||
| { | ||
| if (!callerData.ContainsKey("Trade")) |
There was a problem hiding this comment.
Should use TryGetValue instead
There was a problem hiding this comment.
I don’t think so, cause it needs ro retrieve Trade object by casting the inner object else it would look like;
object value;
Trade trade;
If (tryGetValye()==false) ...
trade = (Trade)value
This is honestly uglier than the check and ger
| { | ||
| val = (string)callerData[key]; | ||
| } | ||
| else if (parameterData.ContainsKey(key)) |
There was a problem hiding this comment.
Should use TryGetValue instead.
There was a problem hiding this comment.
Yes you are right, I’ll change it
| protected float? GetFloatParam(string key, bool require = true) | ||
| { | ||
| float? val = null; | ||
| if (callerData != null && callerData.ContainsKey(key)) |
There was a problem hiding this comment.
Should use TryGetValue instead.
| protected int? GetIntParam(string key, bool require = true) | ||
| { | ||
| int? val = null; | ||
| if (callerData != null && callerData.ContainsKey(key)) |
There was a problem hiding this comment.
Should use TryGetValue instead.
| protected bool? GetBoolParam(string key, bool require = true) | ||
| { | ||
| bool? val = null; | ||
| if (callerData != null && callerData.ContainsKey(key)) |
There was a problem hiding this comment.
Should use TryGetValue instead.
| protected object[] GetObjectArray(string key, bool require = true, params char[] separators) | ||
| { | ||
| object[] res = null; | ||
| if (callerData.ContainsKey(key)) |
There was a problem hiding this comment.
Should use TryGetValue instead.
| protected string[] GetStringArray(string key, bool require = true, params char[] separators) | ||
| { | ||
| string[] res = null; | ||
| if (callerData.ContainsKey(key)) |
There was a problem hiding this comment.
Should use TryGetValue instead.
|
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 That would also remove a lot of the reflection and runtime type inference and in general lead to cleaner code. |
|
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. |
|
@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 |
|
@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) |
|
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. |
|
@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. 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: 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. |
| /// <returns></returns> | ||
| public static DialogBoxManager CreateDialogBoxManager(GameObject parent) | ||
| { | ||
| GameObject root = new GameObject("Dialog Boxes", typeof(RectTransform)); |
There was a problem hiding this comment.
I think we can use nested canvas here for better performance. When any child element changes(size, position etc.) on canvas, it dirties the whole Canvas.
For example Graphic Component calls those below.
public virtual void SetAllDirty()
{
SetLayoutDirty();
SetVerticesDirty();
SetMaterialDirty();
}
public virtual void SetLayoutDirty()
{
if (!IsActive())
return;
LayoutRebuilder.MarkLayoutForRebuild(rectTransform);
if (m_OnDirtyLayoutCallback != null)
m_OnDirtyLayoutCallback();
}
public virtual void SetVerticesDirty()
{
if (!IsActive())
return;
m_VertsDirty = true;
CanvasUpdateRegistry.RegisterCanvasElementForGraphicRebuild(this);
if (m_OnDirtyVertsCallback != null)
m_OnDirtyVertsCallback();
}
public virtual void SetMaterialDirty()
{
if (!IsActive())
return;
m_MaterialDirty = true;
CanvasUpdateRegistry.RegisterCanvasElementForGraphicRebuild(this);
if (m_OnDirtyMaterialCallback != null)
m_OnDirtyMaterialCallback();
}There was a problem hiding this comment.
For more information:
Canvas
UI optimization tips
|
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. |
…f the API in terms of IFunctions, still needs some work but mostly done
|
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 :). |
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
What does this PR intend to do
What does this PR intend not to do
List of changes
Todo
Example of changes
To
Side note: why are callbacks better than IEnumerators here?
PromptOrInfo) where as the old system was hard stuck on only one copy.Images
Since someone almost always asks to see what the changes visually look like here they are;