search

NVlabs / FPSci

Aim Training Experiments
Other
70 stars 23 forks source link

Improve Banner #314

Closed bboudaoud-nv closed 3 years ago

bboudaoud-nv commented 3 years ago

This branch updates the banner to improve configuration and view. Notably it adds:

These changes are related to the changes requested in #313.

jspjutNV commented 3 years ago

Am I correct in my reading of this that it doesn't do anything to support different types of scores as requested by #313 and instead is just about displaying the current scoring mechanism in a few different ways? Just checking so we know to be sure to keep #313 open if we merge this PR.

The mention of a "banner" also reminds me that we should probably do a better job describing what the banner is in the documentation. A labeled screenshot might be nice.

The bannerShowTime variable makes me thing that it's setting when, and for how long to show the banner, but it looks like the variable actually controls how to format the countdown (or up) timer. I'd propose the following variable name changes:

I recognize this would break some existing configs using showBanner so we might want to add a warning message if showBanner is found in a config.

bboudaoud-nv commented 3 years ago

Correct, this PR isn't linked to/doesn't close #313 as it does not propose a solution for that issue, just cleans up general banner behavior. I believe we should create a separate PR to deal with #313 once we decide on a more formal design for scoring.

I agree that adding a screenshot to document the banner is a good idea. This screen shot should probably include labels of the time, progress, and score fields.

We use the show[X] convention is fairly widely in FPSci config parameter names. The showHUD, showAmmo, showPlayerHealth, showReferenceTarget, showCursor, etc parameters is what showBanner/bannerShow[X] naming was based upon, so if do choose to change this we should likely change everything. I believe the only place we currently use [x]Enable is the logEnable field (which makes sense since it is not a visual element).

I do think it is a good idea to move all of our config fields to names that start with banner if possible, grouping them logically if serialized out. I also agree that bannerTimerMode is an improvement over bannerShowTime and we should make this change. Otherwise I don't have strong preferences on naming for these fields.

jspjutNV commented 3 years ago

Cool, your reasoning makes sense. Let's just switch to bannerTimerMode and keep the other names how they were.

jspjutNV commented 3 years ago

I just tried to run this PR on my experiment config that works with master and other branches, and this one crashes on startup with an access violation in line 181 of Logger.cpp. I can't explain what is going wrong, or why these specific changes would cause this error (on a cursory read of them they shouldn't), but this is going to need more investigation.

bboudaoud-nv commented 3 years ago

@jspjutNV can you provide the exception message and your config file to review?

This exception seems odd since nothing in this branch should impact the logger at all...

jspjutNV commented 3 years ago

Repeating my comment from #319 since it's probably the same bug I'm hitting.

I believe I've gotten it to build by changing line 182 in Logger.cpp to make the first "'" literal into a G3D::String with the following.

      String("'") + sessConfig->id + "'",

Since there's a bunch more of the same implicit conversion to G3D strings to get the G3D + operators to work, it seems like we should probably be explicitly making those strings into G3D::String types. I cannot explain why this only breaks on a couple branches, some of which were working for me before, but doesn't break on the master branch.