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 JSON API for mass deleting actors #382

Merged
merged 11 commits into from
Dec 23, 2024

Conversation

MayberryZoom
Copy link
Contributor

@MayberryZoom MayberryZoom commented Dec 19, 2024

Adds a JSON API for mass deleting actors.

Per scenario/actor layer, actors can be mass deleted by the following conditions:

  • all
  • all within the specified groups
  • all not within the specified groups

Additionally, exceptions can be made to not delete specific actors.

This is mainly useful for creating the lights out effect. This will be used for an RDV option to remove all lights, and can be useful for ROM hack creators to remove the lights in specific scenarios/rooms while selectively leaving specific actors untouched. There's probably some niche use case for the entities/sounds layers, but there was no reason to restrict this to only lights, so I left it open for any layer.

Also fixes a bug where when the door patcher renamed a shield, it would leave the name of the original shield in the actor groups. For the mass delete feature this created an issue where if a group contained a shield rando had modified, it would try to delete an actor that doesn't exist and error, so I decided it was best to fix it in this PR.

Copy link
Member

@henriquegemignani henriquegemignani left a comment

Choose a reason for hiding this comment

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

Can you add unit tests for mass_delete_actors.py as a whole?

For the helper methods, should be doable if you mock scenario.all_actors_in_actor_layer/scenario.get_actor_group to return some data, and then you check to_remove/to_keep has what you expected. Meanwhile if you mock the helpers and .remove_entity it should be fairly simple to test mass_delete_actors as well

src/open_dread_rando/files/schema.json Show resolved Hide resolved
src/open_dread_rando/files/schema.json Outdated Show resolved Hide resolved
Copy link

codecov bot commented Dec 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.98%. Comparing base (e6a7f8f) to head (fafe819).
Report is 12 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #382      +/-   ##
==========================================
+ Coverage   91.77%   91.98%   +0.20%     
==========================================
  Files          37       38       +1     
  Lines        2104     2159      +55     
==========================================
+ Hits         1931     1986      +55     
  Misses        173      173              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MayberryZoom
Copy link
Contributor Author

I added the test, should get full coverage from that part. I also added a PatcherEditor fixture, which we can probably use in other places later on.

I did end up adding return values to the helper functions, but not because I wanted to use it for the test. I ran it a few times and for some reason it was slightly faster? I know for a fact Python passes sets via reference, not value, so I have no idea what would be so expensive about passing in the original set. To be fair, It could be something weird with my machine, and I only ran each method a few times then eyeballed it, so maybe it's not actually faster. It was in the range of two digits of milliseconds if I remember correctly though (something like 30ms), so that seems unlikely.

I definitely feel like this should be slower since it's just extra operations, but I guess I won't complain about a measured speed increase. And it also made the function arguments a little cleaner, which is probably more important in any case.

@henriquegemignani henriquegemignani added this pull request to the merge queue Dec 23, 2024
Merged via the queue into randovania:main with commit 96850d5 Dec 23, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants