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

Add Piecharts (gd 4.1) #146

Merged
merged 4 commits into from
Aug 17, 2023
Merged

Add Piecharts (gd 4.1) #146

merged 4 commits into from
Aug 17, 2023

Conversation

Nemrav
Copy link
Contributor

@Nemrav Nemrav commented Jul 29, 2023

  • Adds Pie/Donut and a concentric donut + pie chart charts, this time as godot 4.1 scenes (upgrade broke previous pr)
  • Adds placeholder piechart to the province overview panel

Pie charts related scripts and nodes are located at src/game/theme/piechart

Areas of concern/known issues:

  • Resources are wasted as a "new" shader is instanced for every pie chart. This is because, for now, "instance uniforms" aren't supported for 2d shaders.
  • To set the parameters of the shader, there is something of variable shadowing/passthrough from the piechart.gd for convenience's sake, due to the above lack of instance uniforms on the chart shader.
  • The tooltip does not always go away when the mouse exits the area. I don't know how to fix this. Custom tooltips are used here to allow for rich text (ie. bolding and underlining the text).

Future improvements:

  • I expect requests for code style improvements
  • looks to be tweaked some more
  • Additions to the RichTooltips

Previous godot 3 version was PR #119

@Nemrav Nemrav mentioned this pull request Jul 29, 2023
@Hop311
Copy link
Contributor

Hop311 commented Aug 14, 2023

As well as Nemrav's great work on pie charts, this PR includes:

Main repo

  • Piecharts are generated at the godot-cpp level (using Nemrav's shader translated into Godot C++) and then cached until next updated. In the future this should be changed to use the overlay textures (and ideally the Godot draw functions)
  • Added province pop type, ideology and culture pie charts, hooked up to receive and display data from the simulation (although ideology doesn't send anything yet because political stuff hasn't been implemented)
  • Added culture/nationality mapmode, for now only showing largest culture per province (this PR doesn't include culture loading, that'll be in the next one)
  • Removed some chained signals for load progress starting, changing, and finishing, as they already have global signals in Events

Sim submodule

  • Created HasIdentifierAndColour (inheriting from both HasIdentifier and HasColour), for use in distribution_t = std::unordered_map<HasIdentifierAndColour const*, float>, the C++-level representation of piechart data. (Might be better to use std::map?). Currently provinces generate distribution_ts for pop type and culture,
  • Replaced a lot of std::string const& args with const std::string_view to reduce unnecessary string copies, including having IdentifierRegistry::get_item_by_identifier search for a std::string_view in a std::string-keyed std::map without needing to create a temporary std::string
  • Clear pops before generating test pops in Map::setup, so that resigning to the main menu and then going back into the game doesn't add on to the pops from the previous session
  • Date::from_string rewritten to take std::string_view argument and std::from_chars to turn parse ints (previously we checked the string was valid then passed it to stoi, hoping it wouldn't throw an exception we hadn't considered). Once Fakebyte's fixed point stuff is merged we could use its string_to_int64 function, if there's any advantage over std::from_chars.
  • Rearranged province building generation to make sure the province's building IdentifierRegistry locks even after encountering an error
  • Added error message for invalid input to Map::set_selected_province
  • Added summary of province and terrain image loading errors (e.g. unrecognised colours or missing provinces) and the option to disable the full errors (very spammy with vic2's province image which contains many mistake pixels and doesn't contain a fair few provinces defined in definition.csv)
  • Added check + error message to prevent adding a Pop to a water province
  • Moved province pop stats updating from a their own individual functions to Province::update_pops, since they'll never be updated in isolation and this way we only need to iterate over the province's pops once per update
  • Fleshed out ProvinceSet with its own add_province, lock, is_locked and reset functions, similar to IdentifierRegistry, updated Region generation to use the new methods and changed Map to use a ProvinceSet to store all water provinces (while still setting a flag in the provinces themselves)
  • Reordered the arguments of various constructors and constructing functions to match the order the variables are actually initialised in (mostly moving identifier and colour to the front)

Copy link
Member

@Spartan322 Spartan322 left a comment

Choose a reason for hiding this comment

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

Approved for now, but I really want to refurbish everything going on here, move things to proper places, use the proper functions, organize behavior out of Utilities and GameSingleton.

@Hop311 Hop311 merged commit aa9961b into OpenVicProject:master Aug 17, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request requirement ui functionality Requirement UIFUN requirement ui Requirement UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants