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

Implement display toggle for zones #71262

Merged
merged 10 commits into from
Jan 29, 2024
Merged

Conversation

PatrikLundell
Copy link
Contributor

@PatrikLundell PatrikLundell commented Jan 25, 2024

Summary

None

Purpose of change

Fix #70992, i.e. implement a display toggle for zones to allow players to select whether to show the zones or not (default off).

By-catch fix of UI bug where the cursor always was placed at the end of the zone list when creating a new zone, even though zones tied to vehicles are placed at the end. Now the cursor ends up at the newly created zone.

Describe the solution

Define one field for each kind of zone with a distinct character tied to the field. Add a toggle to the zone UI to allow the zones to be displayed and hidden.

Reorganized the zones between the two files defining them to make somewhat more sense, plus sorted them in alphabetic order.

The new display fields are placed in a separate file.

Updated the JSON format for zones to have a mandatory JSON field referencing a field object (unfortunate English word overload there...).

Encountered and fixed bug where removal of a field ended up in an eternal loop because the iterator wasn't progressed when the field to be removed wasn't the first one.

Describe alternatives you've considered

The glyphs assigned to the fields associated to the various zones could have been done differently. There will probably be suggestions. After all, this is almost a perfect bike shed issue (but I'm not saying suggestions won't be valid: the selection is closer to arbitrary than perfect).

There's a zone that's partially implemented, but doesn't show up in the list of available zones, LOOT_ITEM_GROUP. I've added it to the new code, but am not going to make any attempt to make it available: I don't know if the associated code is implemented, and don't want to deal with trying to make it work if it's buggy.

Testing

  • Removing .../config/keybindings.json and starting the game.
  • Toggle display of all existing zones (not that many, but some at a different Z level in a different world tile).
  • Verified they all showed up apart from the one under an electric forge (fields under vehicles aren't displayed, and appliances are counted as vehicles). In contrast, the one over the anvil (furniture) did appear.
  • Bound a new key to generate a new keybindings.json file and verified both that the file was generated and that the toggle key binding still works.
  • Defined a zone of each kind, displaying them as they're created.
  • Created zone tied to vehicle and received a popup when trying to toggle its display (Fields hidden under vehicles anyway).
  • Constructed 3 different zones on top of each other and showed their display stacked and that they could be toggled individually.
  • Constructed overlapping basecamp storage zones and showed they could be toggled individually without gouging out parts of the other zone.
  • Created a huge tree chopping zone and verified it could be toggled and verified the whole area was marked with the display.

Screen shot at the end of testing (the percentage signs are not zone markers, but incomplete disassembly tasks):
Screenshot (327)

Additional context

@PatrikLundell PatrikLundell marked this pull request as draft January 25, 2024 21:53
@github-actions github-actions bot added [JSON] Changes (can be) made in JSON [C++] Changes (can be) made in C++. Previously named `Code` Fields / Furniture / Terrain / Traps Objects that are part of the map or its features. <Bugfix> This is a fix for a bug (or closes open issue) astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions labels Jan 25, 2024
data/json/field_type.json Outdated Show resolved Hide resolved
src/clzones.cpp Outdated Show resolved Hide resolved
src/clzones.cpp Show resolved Hide resolved
src/clzones.cpp Show resolved Hide resolved
src/clzones.cpp Outdated Show resolved Hide resolved
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Jan 26, 2024
@PatrikLundell
Copy link
Contributor Author

I don't know how to resolve the reported error in pre existing code:
Clang-tidy 16 / build (src) (pull_request) Failing after 120m:

Error: /home/runner/work/Cataclysm-DDA/Cataclysm-DDA/src/clzones.cpp:1737:9: error: Called C++ object pointer is null [clang-analyzer-core.CallAndMessage,-warnings-as-errors]
dynamic_cast<loot_options *>( &*options )->set_mark( filter );
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Is it a case of not recognizing a custom pointer as being a pointer (again: had a previous case where I was demanded to change existing code to make a reference to a custom pointer (and then de-referencing it when used)?

@PatrikLundell PatrikLundell marked this pull request as ready for review January 27, 2024 18:57
@Qrox
Copy link
Contributor

Qrox commented Jan 28, 2024

I think it's because clang-tidy does not know that these zone types can only have loot_option options, so it thinks the dynamic cast can result in a nullptr. Not sure why it did not report the warning before though. Testing the casted pointer is null before dereferencing it should fix it.

@PatrikLundell PatrikLundell changed the title [WIP] Implement display toggle for zones Implement display toggle for zones Jan 28, 2024
@Maleclypse Maleclypse merged commit b3a2e0d into CleverRaven:master Jan 29, 2024
26 of 28 checks passed
rtxyd pushed a commit to rtxyd/Cataclysm-DDA that referenced this pull request Jan 29, 2024
rtxyd added a commit to rtxyd/Cataclysm-DDA that referenced this pull request Jan 29, 2024
@PatrikLundell PatrikLundell deleted the show_zone branch January 29, 2024 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Fields / Furniture / Terrain / Traps Objects that are part of the map or its features. [JSON] Changes (can be) made in JSON json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Display the location of zones with a toggle
3 participants