Skip to content
This repository has been archived by the owner on Dec 18, 2017. It is now read-only.

New Feature/Toolset: Paradox Bar! #514

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

New Feature/Toolset: Paradox Bar! #514

wants to merge 20 commits into from

Conversation

chaorace
Copy link
Member

@chaorace chaorace commented Apr 30, 2017

What is this?

Okay, so there's the name... It's basically an imitation of the the notification scheme found in the grand-strat games published by Paradox, like Crusader Kings, Europa Universalis, and Stellaris. It looks like this if you're drawing a blank.

Right, so that's the idea, implementing a saner notification scheme that's easier to read. How far along is this pie-in-the-sky dream? It's complete! So, why is this a PR instead of just going direct-to-game? Well... there's only two notifications implemented so far, and they're both purely for debugging purposes. That being said, the internals have been tested extensively and the "API" of this element has been engineered specifically for robustness, flexibility, and ease of use. It's ready for people to start throwing things at it now!

Seeing as we've run short on low-hanging fruit recently, which has dried out the flow of commits somewhat, I've decided to put this PR up as an additional avenue for both the @CQUI-Org/feature-team and outside contributors to quickly and easily extend CQUI.

So, what's the plan?

The plan is to implement notifications that do not exist currently into this new notification system. Once we're confident with the number of notifications implemented, we'll soft-launch this while keeping the old notification system. This way we'll have a chance to gather some user feedback on performance, bugs, and usability without really risking bricking up the notifications that already exist in the base game. After that, we'll see about moving over all notifications to this new notification platform, along with implementing much-requested features like snoozing notifications and muting specific notification groups

Why abandon the existing notification bar?

It's difficult to mod, plain and simple! The difficulty present in extending the existing notification system makes it a very unpleasant proposition and is the reason we've been unable to tackle the many existing requests for adding new notifications. It's at the point where we had to instead hijack the gossip popups instead, even when they weren't necessarily the best choice. Moving over to our own custom solution also comes with the added benefit of being able to add notification stacking, a much requested feature.

Beta

It's been too long, I've been putting off the beta tag since November. Let's finish up this last feature and finally release the first beta build 😎

This creates a framework for placing new elements relative to where the
vanilla UI elements appear without being particularly invasive
This method is called the paradoxbar. So far only two test notification
types exist "debug" and "debug2". Keep in mind that this tool is very
flexible and quite capable of both one-off notification types as well as
template-based notifications. Custom behavior should be relatively
simple to implement and each notification falls into a category with
others that fall in the same grouping
Copy link

@SpaceOgre SpaceOgre left a comment

Choose a reason for hiding this comment

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

Did a fast pass over the code without testing it. Will do that when I get more time.

local paradoxBarStock = {
["debug"] = {"Debug", "Controls_Circle", "This is a debug tooltip", "Dbg", {paradoxBarFuncs["debugPrint"], paradoxBarBundles["standard"]}},
["debug2"] = {"Debug2", "Controls_Circle", "This is a debug tooltip", "Dbg2", {paradoxBarFuncs["debugPrint"], paradoxBarBundles["standard"]}}
};

Choose a reason for hiding this comment

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

I feel like this should be done with key/value pairs instead of indexes. It is much easier to get the meaning when you read the code further down:
instance.Text:SetText(defaults[4]); vs instance.Text:SetText(defaults.Text);
And it makes it safer to not mess it up when adding new properties to the table/array, if you add a new value before the text then you have change all usages of defaults[4] to defaults[5].

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll be implementing this suggestion now, thanks for the heads up 👍

instance.Text:SetText(defaults[4]);
end
if(defaults[5] and not funcs) then
funcs = defaults[5]

Choose a reason for hiding this comment

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

This will always overwrite the passed funcs if there is a default. It might be what you are after but it does not follow the pattern that the rest of the function parameters are following in that passed parameters is used instead of default.

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, this behavior is different because we want overriding a template to allow full control of the notification. In the scenario where we merely append new functions to the list, it would only be possible to extend a given template's behavior, as opposed to completely manipulate it. This comes with the consequence of being more verbose, but I feel more control is better if you're in a situation requiring that you create a custom notification with one-off behavior in the first place

Choose a reason for hiding this comment

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

Not sure I follow what you mean. Is the idea that calls to AddNotification only should pass funcs if it is a one off, ie if the passed ID does not have defaults?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what I mean, yes. If you plan to be using a specific type of notification more often than just once, it is advisable to compose a template first.

That being said, that's why these behavior bundles are included, so that function composition is more convenient and idiomatic for the purposes of composing those one-offs.


--Applies defaults where appropriate
if(defaults) then
if(defaults[2]) then

Choose a reason for hiding this comment

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

A small thing and I'm not sure how expensive these UI functions are (I guess it does nothing until a new render pass is done) But I'm not a fan of calling a function that will be called again just to overwrite the last call.

Here we already know if icon is passed so maybe change the check to if(defaults[2] and not icon) or rewrite it in some other way.

Copy link
Member Author

Choose a reason for hiding this comment

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

This isn't called every render pass, this function is meant to be called once each time a new notification event is thrown. As for the UI functions starting at line 55, these only get called once when a new notification group is being created. These are relatively heavy functions, but no more potential for a performance hit compared to what you'd see when multiple vanilla notifications are created.

Copy link
Member Author

Choose a reason for hiding this comment

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

Concerning your second point, I'm struggling to figure out what you mean, could you elaborate?

Choose a reason for hiding this comment

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

About the second point: if you call AddNotification with an icon and the ID have defaults you will call
instance.Icon:SetTexture(defaults["icon"]); and then instance.Icon:SetTexture(icon);
Making the first call unnecessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose that is somewhat inefficient. Alright, I'll patch that up

This change should make creating partiallyt defined templates easier and
hopefully also make the source easier to understand
This commit prevents unnecessary use of the default values whenever an
overriding value is present, improving efficiency somewhat
@chaorace
Copy link
Member Author

chaorace commented May 1, 2017

Heads up, I'm working on improving the LOC text support for notifications, so please hold off on anything related to this PR for a few hours while I polish this up, changes will be breaking

This change bundles all properties into a single "props" table. The goal of this change is to make the method somewhat more wieldy while also preventing future changes from breaking existing method calls, since this new structure isn't strictly dependant upon the order of the property
This resolves an issue where the debug log printout was incorrect, as well as preventing "Blah" from appearing in a notification if no string is specified
@chaorace
Copy link
Member Author

chaorace commented May 1, 2017

Okay, I've overhauled the AddNotification method somewhat, this should be the last truly breaking change, since the new way of providing properties is ordering agnostic

@chaorace
Copy link
Member Author

chaorace commented May 1, 2017

Also added better LOC support so it should be considerably easier to create a template string now while filling in the blanks at time of instantiation with a property override upon the new ttprop table

@SpaceOgre
Copy link

SpaceOgre commented May 1, 2017

Got some time to try it out in civ today, and have some comments.

Lets say you create 3 notifications in the same group and remove one and then add new ones the position gets messed upp:
image

The animations look a bit yanky, especially when removing a notification. Right now it goes over the top bar might look better if it started to fade out as well or if it went under the top bar.

It would be nice to be able to remove an entire group with one click, so maybe add an X button or something like that.

When all buttons are added to the worldtracker the notifications overlap the hide worldtracker checkbox.
image

Also I'm wondering if not AddNotfication might need another parameter or so. Say we want to create a notification for city growth, this notification should on left click take you to the city in question. To do this we need to have a city id or something like it. Otherwise we would need to pass a function for in the func parameter and then we can't have a default which we want to still have right click to remove and so on.
Another thing I just thought about, we need to make sure that a notification is only shown for the player it concerns, this is very important for MP but also for SP since I don't want notifications about AI stuff.

Otherwise I think the look is spot on! Great work.

@chaorace
Copy link
Member Author

chaorace commented May 2, 2017

Yeah, in terms of the animation I wasn't entirely happy with it, but I was hoping it was just because I was being critical, now that I have some feedback I'll look into smoothing it out a bit. I'll probably just swap it out for a alpha transition instead, because I think that will bypass the second visual issue you describe. Speaking of appearance, do you guys think it looks better without the background banners? I'm very back-and-forth on the subject

As for the parameter suggestion, the params table effectively allows passing whatever arbitrary data you want. The params table is accessible from any given func as the third parameter being passed to it, so it should be a relatively simple proposition with the tools already available.

I'll see about adding a close button to the group instance

I don't think it's possible to design a foolproof safety against notifications showing for the wrong player. Really, it's up to whomever is programming a specific notification to filter events so that only relevant ones eventually create a notification.

@chaorace
Copy link
Member Author

chaorace commented May 2, 2017

Ah, the weird animation behavior has to do with some instancemanager behavior I did not expect!

As it turns out, releasing an asset from a given IM just hides and disables the element. The created instance lives on in memory and will return next time a new instance is needed. This leads to some funny side-effects, like the janky positioning you noticed, if you aren't careful to restore any mutable states to their defaults at the time of summoning

This commit switches the sliding animation to a simpler alpha animation,
as well as fixing a bug related to mutable states persisting with newly
summoned notification instances. This commit also fixes a minor issue
which could cause the notification overlay to overlap with the
worldtracker checkbox
@SpaceOgre
Copy link

SpaceOgre commented May 2, 2017

Overlooked the prop parameter, this will do what I asked very well.

I like the background banner, it feels like it matches up nicely with the civ icons on the right.

Edit:
Just thought about something else: icons from atlases. A lot of the vanilla icons are stored in atlases and as such x and y offsets are needed in SetTexture eg:

local textureOffsetX, textureOffsetY, textureSheet = IconManager:FindIconAtlas(iconName,38);
if (textureOffsetX ~= nil) then
 Controls.ReportsImage:SetTexture( textureOffsetX, textureOffsetY, textureSheet );
end

@JHCD
Copy link

JHCD commented May 2, 2017

Do I have to activate somthing? Nothing happens here...

@SpaceOgre
Copy link

@JHCD You have to use Firetuner to test it, atm it is not hooked up to any events and only have debug notifications.

@JHCD
Copy link

JHCD commented May 2, 2017

Okay, no idea how that works... Use Firetuner only for logging...

@SpaceOgre
Copy link

I can probably fix some informative screen shots later today.

@SpaceOgre
Copy link

First you want to select the correct state, in this case cqui_toplayer:
image

Now you have access to all functions declared in that state. So we can write AddNotification("debug") in the input textbar:
image
Press enter and you should see the notification.

@SpaceOgre
Copy link

SpaceOgre commented May 3, 2017

Did some testing with icons and have some more feedback:

  • The last notifcation should have as much margin bottom as the first one have at the top
    image
  • To get the icon in the screen shot above I had to change SetTexture to SetIcon, but when I did that Controls_Circle did not work. I think we might need a more complex icon handling system for this since Icons/images are handled very differently all over vanilla and these icons are nice to use.
  • There seems to be some weird line at the bottom now, it was not there before.
    image

@chaorace
Copy link
Member Author

chaorace commented May 4, 2017

I'll see about possibly autodetecting the image/icon difference. Worst case scenario: I think we could get away with just attempting both functions blindly and swallowing any exceptions. The background banner needs to be at least a certain width, otherwise the transparent textures on the bottom overlap and look ugly like that. I'll look into widening those banners slightly, as well as nudging the icons a bit up relative to the banner

chaorace added 8 commits May 4, 2017 20:29
This change introduces the ability to include offset data in the "icon"
field, which is necessary in order to use textures from the vanilla
atlases
Previously, it was only possible to provide textures, now either should
work without any extra frills required
Notification icons are now more centered both vertically and
horizontally: http://i.imgur.com/wQeQEis.png

This commit also fixes the issue where the shadow textures underneath
the banner would overlap slightly, creating an ugly effect
Now definitions can be defined against already existing stock types, allowing for more concise definitions if there are already similar notifications in use
AddNotification is now much more flexible in terms of handling arbitrary properties and only demands manual handling for certain special cases. Group is now assumed to be handled as part of the preset attached to the ID, group can still be overridden via the props table
These properties allow for controlling the color and stretching of the icons used in notifications
The game doesn't actually have separate events for these two happenings, so I've implemented a hacky heuristic that does the job in almost all cases (hopefully)
@chaorace
Copy link
Member Author

Okay, I've further tweaked the AddNotification method and also changed how new stock notification types are added so that they can be extended if a similar type already exists. These are breaking changes, so sorry about that!

Aside from that, you can see two new notifications have been implemented! Now you'll be alerted whenever a city grows or shrinks. Unfortunately, the game isn't very descriptive about what happens when a city population changes, so this relies upon a nasty heuristic. Keep an eye out for this notification since my personal testing can only do so much!

@SpaceOgre
Copy link

I'm traveling this weekend and we are painting in the computer room so won't be able to test for a while.

@chaorace chaorace mentioned this pull request May 14, 2017
@SpaceOgre
Copy link

SpaceOgre commented Jul 6, 2017

Started a new game today with this on and have found some bugs/problems:

  1. When I first settled my city the bar notified me that the city population had shrunk. If possible it might be best to just skip a notification for this scenario since I know I just founded a city.
    bar_city_founded_bug
  2. I found a natural wonder with the bar still up and it does not hide like the rest of the UI:
    bar_shown_wonder_bug
  3. When I built a settler I got a notification that the population had increased instead of decreased.
  4. The bar is visible when dialog with other civ is open:
    image
  5. When conquering a city I get two notifications, on for shrinking and one for growth
  6. The bar still covers part of the worldtracker buttons:
    image

@chaorace
Copy link
Member Author

chaorace commented Jul 8, 2017

Thanks for testing!

The initial notifications are indeed a bug, just one I haven't gotten around to addressing since it's helpful for testing purposes.

The second issue is because toplayer doesn't respect BulkHide yet, but because the worldtracker keeps bugging us, I think I'll just move the notifications to the same "layer" as the worldtracker, bypassing the positioning and hiding issues entirely

Third and fifth issues are caused by the unfortunate fact that there's only a populationchanged event (we have to infer if this is a growth or shrink event by the city growth, which isn't always accurate, like here), the best solution to this specific issue is probably ignoring growth/shrink events that happen during your turn, since we're mostly concerned with notifying about growth/shrink events the player doesn't directly cause.

@SpaceOgre
Copy link

True, I wish the API:s where better :(

I guess we need to store that information our self if we want a better check. We could store current population for each city on end turn, then we can check against that. Or something like it, but it would take a bit of work ofc.

One more thing I wonder how it would work when a city gets nuked (not that the AI have nuked me yet but I guess it could happen).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants