Show progress in contrib level menus #2040

Narre · 2021-12-28T22:10:43Z

I fixed the feature that was broken for so long it got removed entirely. And I made it better.

…lution)

…dmap)

bruhmoent · 2021-12-28T22:17:31Z

I love it!

Narre · 2021-12-28T22:30:22Z

Feedback please! Should I remove the worlds[i]->get_title() == wm_title in

?? I'm not sure if it's a good idea to separate the progress indicator of the add-on and the worldmap just because their titles don't match.

… worldmap

mrkubax10 · 2021-12-29T08:30:19Z

Also maybe don't show progress for levelsets

Narre · 2021-12-29T11:54:59Z

Why though? Is there a particular reason why the progress of levelsets shouldn't be shown? There is no rule saying that all non-test levels must be in a worldmap, it's just that most people choose to create a worldmap for their levels, but there's nothing stopping them from creating a levelset of playable levels, and then the progress makes sense there.

Narre · 2021-12-29T11:57:43Z

Also I decided to remove that check that would separate world and worldmaps if their titles didn't match since I don't think it makes much sense. I originally had that in place before I implemented the total world level count so I couldn't check if the numbers were the same but now I feel it makes things less intuitive and unnecessary.

Semphriss · 2022-01-03T07:36:30Z

data/levels/christmas2016/worldmap.stwm

+  (name (_ "Christmas Special"))
  (author "Carsten Wirtz")


Were the name changes intended to be part of this PR? (They weren't mentioned in the PR desc)

There's also Community Island 2020 -> Community Island and Haloween 2014 -> Halloween Special.

I renamed them because I originally had that check in place that would show both names from info and the name of the worldmap if the titles didn't match even if the level counts did. I removed that check now (that's what I wanted feedback about, but ultimately decided was pointless and removed it), so the titles don't need to be changed anymore. I still think it's better this way (the worldmap titles are consistent with the title in the info file), but if you think I should change them back, I will. These don't show up anywhere in the game anyway.

As a matter of principle, it would be preferable to discuss add-on renamings with the community, just to ensure that the new name makes sense and that everybody is fine with it.

src/supertux/menu/sorted_contrib_menu.cpp

Semphriss · 2022-01-03T07:41:38Z

src/supertux/menu/sorted_contrib_menu.cpp

      std::string title_str;
+      const auto savegame = Savegame::from_file(worlds[i]->get_savegame_filename());
      if (worlds[i]->is_levelset())
-        title_str = "[" + worlds[i]->get_title() + "]";
+      {
+        uint32_t level_count = 0, solved_count = 0;
+        const auto& state = savegame->get_levelset_state(worlds[i]->get_basedir());
+        for (const auto& level_state : state.level_states)
+        {
+          if (level_state.filename.empty() || level_state.filename.back() == '~') continue;
+          if (level_state.solved) ++solved_count;
+          ++level_count;
+        }
+        if (!level_count)
+        {
+          title_str = str(boost::format(_("[%s] *NEW*")) % worlds[i]->get_title());
+        }
+        else
+        {
+          /* This is translatable since RTL languages may prefer to put the progress
+             info to the left of the title */
+          title_str = str(boost::format(_("[%s] (%u/%u; %u%%)")) % worlds[i]->get_title() %
+                          solved_count % level_count % (100 * solved_count / level_count));
+        }
+      }
      else
-        title_str = worlds[i]->get_title();
+      {
+        uint32_t island_level_count = 0, island_solved_count = 0,
+                 world_level_count = 0,  world_solved_count = 0;
+        auto wm_filename = savegame->get_player_status().last_worldmap;


Please add some empty lines in here, it feels cramped 😄

Semphriss · 2022-01-03T07:42:26Z

src/supertux/menu/sorted_contrib_menu.cpp

+          else
+          {
+            /* This is translatable since RTL languages may prefer to put the progress
+               info to the left of the title */
+            title_str = str(boost::format(_("%s (%u/%u; %u%%) - %s (%u/%u; %u%%)")) % worlds[i]->get_title() % 
+                            world_solved_count % world_level_count % (100 * world_solved_count / world_level_count) %
+                            wm_title % island_solved_count % island_level_count %
+                            (island_level_count ? (100 * island_solved_count / island_level_count) : 100));
+          }


Is there a reason to duplicate the count for a world and for a worldmap? Isn't the world's stats enough?

I originally thought it may be enough the other way round :D

I think having both counts is good for bigger worlds with more than one worldmap, like

For worlds which only have one worldmap (ie the level count in the worldmap and the world are identical), only one will show.

Semphriss · 2022-01-03T07:45:49Z

src/supertux/savegame.cpp

+  std::string path;
+  if (name[0] == '/') {
+    path = name;
+  }
+  else {
+    path = "/" + name;
+  }


This check seems needless - it should be on the caller to ensure that there is a leading slash if a leading slash is necessary. (Also, it would be preferable to use the FileUtils where possible.)

Semphriss · 2022-01-03T07:48:29Z

src/supertux/player_status.hpp

+  std::string last_worldmap_title; /**< the title of the last played worldmap */



Marking this in case the worldmap data gets removed, this variable will be unnecessary.

Semphriss

There are a few superfluous comments, and I'd like to bring up the topic of renaming the add-ons with the community before finalizing this. Requesting changes only to remember about it, but once those are done, this PR will be clean and ready to merge.

Semphriss · 2022-01-05T02:04:25Z

src/supertux/menu/sorted_contrib_menu.cpp

+          // This is translatable since RTL languages may prefer to put the progress
+          // info to the left of the title


This should be obvious from the context; this comment is not required.

Semphriss · 2022-01-05T02:05:39Z

src/supertux/menu/sorted_contrib_menu.cpp

+            // This is translatable since RTL languages may prefer to put the progress
+            // info to the left of the title


Semphriss · 2022-01-05T02:05:47Z

src/supertux/menu/sorted_contrib_menu.cpp

+          {
+            // This is translatable since RTL languages may prefer to put the progress
+            // info to the left of the title


Narre · 2022-01-05T18:52:20Z

Those titles aren't the names of the Add-Ons (those are in the info file). These are the names of the worldmaps (the stwm "levels"), which don't show up anywhere in the game. I just made them consistent with the Add-On names in info.

Semphriss · 2022-01-11T15:21:52Z

src/supertux/menu/sorted_contrib_menu.cpp

+#include "boost/format.hpp"
+#include "gui/menu_manager.hpp"


One small detail: There should be an empty line between headers from dependencies (SDL, Physfs, Boost, etc.) and headers from the source directory

Zwatotem

Please apply the suggestions that I highlighted. (Or inform me why they shouldn't be)

Zwatotem · 2022-02-06T17:40:22Z

src/supertux/menu/sorted_contrib_menu.cpp

+      {
+        uint32_t level_count = 0, solved_count = 0;
+        const auto& state = savegame->get_levelset_state(worlds[i]->get_basedir());
+
+        for (const auto& level_state : state.level_states)
+        {
+          if (level_state.filename.empty() || level_state.filename.back() == '~') continue;
+          if (level_state.solved) ++solved_count;
+          ++level_count;
+        }


I feel like this counting logic doesn't belong here, and could be better reused, if it was moved to a dedicated LevelsetState method.

Zwatotem · 2022-02-06T17:43:06Z

src/supertux/menu/sorted_contrib_menu.cpp

+        uint32_t island_level_count = 0, island_solved_count = 0,
+                 world_level_count = 0,  world_solved_count = 0;
+        std::string wm_filename = savegame->get_player_status().last_worldmap;
+
+        if (wm_filename.empty())
+          wm_filename = worlds[i]->get_worldmap_filename();
+
+        wm_filename = "/" + wm_filename;
+
+        const auto& state = savegame->get_worldmap_state(wm_filename);
+        for (const auto& level_state : state.level_states)
+        {
+          if (level_state.filename.empty()) continue;
+          if (level_state.solved) ++island_solved_count;
+          ++island_level_count;
+        }
+
+        const auto levelset = std::unique_ptr<Levelset>(new Levelset(worlds[i]->get_basedir(), true));
+        world_level_count = levelset->get_num_levels();
+        const auto world_list = savegame->get_worldmaps();
+
+        for (const auto& world : world_list)
+        {
+          const auto& world_state = savegame->get_worldmap_state(world);
+          for (const auto& level_state : world_state.level_states)
+          {
+            if (level_state.filename.empty()) continue;
+            if (level_state.solved) ++world_solved_count;
+          }
+        }


Same as before, but for WorldmapState as well. The code is being repeated at this point.

src/supertux/menu/sorted_contrib_menu.cpp

Semphriss · 2022-03-09T19:11:15Z

src/supertux/menu/sorted_contrib_menu.cpp

+            title_str = str(boost::format(_("%s (%u/%u; %u%%) - %s (%u/%u; %u%%)")) % worlds[i]->get_title() % 
+                            world_level_count.first % world_level_count.second %
+                            (100 * world_level_count.first / world_level_count.second) %
+                            wm_title % island_level_count.first % island_level_count.second %
+                            percentage);


That line is quite long; is it necessary that both the world and the worldmap get their info there? I doubt most level designers would care to set both to significantly different values. It would be better to only retain the world info; island-specific info is unnecessary and better fit inside the world itself if necessary.

mrkubax10 · 2022-08-12T09:06:32Z

What is left to do for this pull request besides resolving merge conflict?

Remove community island (as it was removed on master)

Narre · 2022-08-14T14:30:19Z

I think I'm (for the most part) done with this PR. However, there is one minor annoyance I'm not sure how to fix and a few design choices that may be worth considering, but can be addressed in another PR.

The minor annoyance is that the title of the current worldmap is saved in the savegame file in the language SuperTux is set to at the time of saving (loading the particular worldmap), and if the language is changed afterwards, the title will only update after reloading the worldmap. This can be fixed by saving the last worldmap title without translation and translating it directly in sorted_contrib_menu.cpp, however, the m_name variable loaded in worldmap.cpp loads the already translated string and I couldn't figure out where it's pulling it from (or rather, how it's pulling it). Maybe using an actual IDE instead of a plain text editor would help...
The design choices include the most obvious one - whether and how (not) to count cutscenes and similar towards the level count. This could be achieved with a bool in the .stwm map, but then the total level count (between all worldmaps) would not see it. This would probably be better addressed separately in another PR since I'm not sure how I would do it and I'm sure there are people better at tackling this than me.
The other design choice I thought about is where to show the progress. Currently, in this PR, the progress is shown the way it used to be before this feature broke (next to the world title), but since this line is kinda long now (because for worlds that consist of more than one worldmap, it shows overall progress as well as the progress in the current worldmap). I thought it may be better to show the progress in another manner, for example, at the bottom of the screen alongside the description, but this would require some discussion and when decided, can be done in a single simple commit.

Narre · 2022-08-14T15:28:58Z

Also, before anyone asks, I removed the Community Island to resolve a merge conflict since it was already removed on master

mrkubax10 · 2022-08-20T08:00:42Z

The other design choice I thought about is where to show the progress. Currently, in this PR, the progress is shown the way it used to be before this feature broke (next to the world title), but since this line is kinda long now (because for worlds that consist of more than one worldmap, it shows overall progress as well as the progress in the current worldmap). I thought it may be better to show the progress in another manner, for example, at the bottom of the screen alongside the description, but this would require some discussion and when decided, can be done in a single simple commit.

Since descriptions aren't usually long I think that moving progress information there is a good idea.

mrkubax10 · 2022-08-20T08:14:35Z

The minor annoyance is that the title of the current worldmap is saved in the savegame file in the language SuperTux is set to at the time of saving (loading the particular worldmap), and if the language is changed afterwards, the title will only update after reloading the worldmap. This can be fixed by saving the last worldmap title without translation and translating it directly in sorted_contrib_menu.cpp, however, the m_name variable loaded in worldmap.cpp loads the already translated string and I couldn't figure out where it's pulling it from (or rather, how it's pulling it). Maybe using an actual IDE instead of a plain text editor would help...

This is because the lisp files can contain translations inside of them. Look at this name entry from data/world1/worldmap.stwm:
(name (_ "Icy Island")) (_ means translated string). If I understand this correctly worldmap name is automatically translated in get method of ReaderMapping: https://github.com/SuperTux/supertux/blob/master/src/util/reader_mapping.cpp#L131, which is called there: https://github.com/SuperTux/supertux/blob/master/src/worldmap/worldmap_parser.cpp#L69

Narre · 2023-07-18T15:26:53Z

I'm closing this since it's going to be replaced by #2349.

Narre added 12 commits December 28, 2021 20:02

show progress in levelsets

1e4e892

Remove debug prints

04a74bc

show progress in worldmaps (WIP)

bc2d9ab

Show the progress of the last worldmap instead of worldmap.stwm

52939eb

Show current worldmap by name instead of file path (WIP, temporary so…

58d29bd

…lution)

Only show worldmap title if it doesn't match addon title and isn't empty

b909c73

Optimise getting worldmap title

57534e0

Add a progress indicator for entire worlds (not just the current worl…

dfbd5f9

…dmap)

Add my name to sorted_contrib_menu.cpp since I now wrote most of it

530213c

Clean up the code

21d2da2

Replace == "" with .empty()

d183fc7

Name official contrib levels' worldmaps consistently with the world name

2385e16

Semphriss self-requested a review December 29, 2021 00:06

Narre added 5 commits December 29, 2021 01:59

Remove unused line

2dbe201

Fix division by zero

56a18ce

Const correctness

45a7f56

Only separate world and worldmap if not all levels are present in the…

4ef2dcd

… worldmap

More const-ness

831ea4a

Semphriss requested changes Jan 3, 2022

View reviewed changes

Code quality

6c3dc29

Semphriss requested changes Jan 5, 2022

View reviewed changes

Remove unnecessary comments [ci skip]

c3ea663

Semphriss requested changes Jan 11, 2022

View reviewed changes

Separate external includes [ci skip]

35c299d

Semphriss approved these changes Feb 6, 2022

View reviewed changes

Zwatotem requested changes Feb 6, 2022

View reviewed changes

Narre added 2 commits February 7, 2022 16:01

Move level counting logic for levelsets to LevelsetState

18901b6

Move level counting logic for worldmaps to WorldmapState

3ad4cdc

Narre requested a review from Zwatotem February 7, 2022 16:01

Semphriss self-requested a review February 8, 2022 18:29

Semphriss reviewed Mar 2, 2022

View reviewed changes

src/supertux/menu/sorted_contrib_menu.cpp Outdated Show resolved Hide resolved

Semphriss reviewed Mar 2, 2022

View reviewed changes

src/supertux/menu/sorted_contrib_menu.cpp Outdated Show resolved Hide resolved

Semphriss reviewed Mar 2, 2022

View reviewed changes

src/supertux/menu/sorted_contrib_menu.cpp Outdated Show resolved Hide resolved

Semphriss reviewed Mar 2, 2022

View reviewed changes

src/supertux/menu/sorted_contrib_menu.cpp Outdated Show resolved Hide resolved

Code quality

e5b9fbd

Semphriss reviewed Mar 9, 2022

View reviewed changes

weluvgoatz mentioned this pull request Aug 14, 2022

World select suggestions #2286

Open

Code quality

30a696d

Remove community island (as it was removed on master)

weluvgoatz added this to the 0.7.0 milestone Oct 7, 2022

Narre closed this Jul 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show progress in contrib level menus #2040

Show progress in contrib level menus #2040

Narre commented Dec 28, 2021 •

edited

Loading

bruhmoent commented Dec 28, 2021

Narre commented Dec 28, 2021

mrkubax10 commented Dec 29, 2021

Narre commented Dec 29, 2021

Narre commented Dec 29, 2021

Semphriss Jan 3, 2022

Narre Jan 3, 2022

Semphriss Jan 5, 2022

Semphriss Jan 3, 2022

Semphriss Jan 3, 2022

Narre Jan 3, 2022

Semphriss Jan 3, 2022

Semphriss Jan 3, 2022

Semphriss left a comment

Semphriss Jan 5, 2022

Semphriss Jan 5, 2022

Semphriss Jan 5, 2022

Narre commented Jan 5, 2022

Semphriss Jan 11, 2022

Zwatotem left a comment

Zwatotem Feb 6, 2022

Zwatotem Feb 6, 2022

Semphriss Mar 9, 2022

mrkubax10 commented Aug 12, 2022

Narre commented Aug 14, 2022

Narre commented Aug 14, 2022

mrkubax10 commented Aug 20, 2022

mrkubax10 commented Aug 20, 2022 •

edited

Loading

Narre commented Jul 18, 2023

		std::string last_worldmap_title; /*< the title of the last played worldmap /

		// This is translatable since RTL languages may prefer to put the progress
		// info to the left of the title

		#include "boost/format.hpp"
		#include "gui/menu_manager.hpp"

Show progress in contrib level menus #2040

Show progress in contrib level menus #2040

Conversation

Narre commented Dec 28, 2021 • edited Loading

bruhmoent commented Dec 28, 2021

Narre commented Dec 28, 2021

mrkubax10 commented Dec 29, 2021

Narre commented Dec 29, 2021

Narre commented Dec 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Semphriss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Narre commented Jan 5, 2022

Choose a reason for hiding this comment

Zwatotem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrkubax10 commented Aug 12, 2022

Narre commented Aug 14, 2022

Narre commented Aug 14, 2022

mrkubax10 commented Aug 20, 2022

mrkubax10 commented Aug 20, 2022 • edited Loading

Narre commented Jul 18, 2023

Narre commented Dec 28, 2021 •

edited

Loading

mrkubax10 commented Aug 20, 2022 •

edited

Loading