Add more methods to BRSA #280

MayberryZoom · 2025-03-02T04:26:40Z

Also achieves full coverage of BRSA and adds docstrings to every method (both new and old)

for more information, see https://pre-commit.ci

codecov · 2025-03-02T04:42:08Z

Codecov Report

Attention: Patch coverage is 97.87234% with 1 line in your changes missing coverage. Please review.

Project coverage is 79.12%. Comparing base (9c60be4) to head (e389c1a).

Files with missing lines	Patch %	Lines
src/mercury_engine_data_structures/formats/brsa.py	97.87%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #280      +/-   ##
==========================================
+ Coverage   78.85%   79.12%   +0.27%     
==========================================
  Files          80       80              
  Lines        4323     4365      +42     
==========================================
+ Hits         3409     3454      +45     
+ Misses        914      911       -3

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

duncathan

if you're returning a Container from an API it should probably have its own type too

MayberryZoom · 2025-03-02T08:04:07Z

That's already used in various places, and it's something I plan to clean up once I finish generating classes for every type. We can leave it as a Container for now imo

MayberryZoom · 2025-03-02T19:40:26Z

To elaborate:
Adding full types for these is beyond the scope of this PR. The only thing we could realistically do for now is add a type alias, but that would provide strictly less information than how it is now. What's being returned is already detailed in the docstring, so type aliasing the return type would only obfuscate what the actual data type is (a Container). For the moment, this is the best way to do this. Imo, this PR isn't worth blocking until those types finish getting generated.

That said, I just remembered I should be wrapping some of those lists in ListContainers, so I'll fix that soon.

for more information, see https://pre-commit.ci

MayberryZoom · 2025-03-03T00:48:29Z

I fixed those ListContainers, and I also raised ValueErrors if the subarea config/setup/charclass group already exists, so this PR should be ready now.

duncathan · 2025-03-04T05:02:23Z

src/mercury_engine_data_structures/formats/brsa.py

+        ignore_camera_offsets: bool = False,
+        charclass_group: str = "No Enemies",
+        camera_ids: list[str] = [],
+        cutscene_ids: list[str] = [],


as a rule, you should never use a mutable value as a default argument

I wanted to show that if it's not provided, it defaults to empty, hence the ternaries below

in that case do cutscene_ids: list[str] | None = None and if cutscene_ids is None: cutscene_ids = [] and drop the ternaries

facepalming at myself for not doing that to begin with. thanks

actually, are the ternaries actually that bad? I feel like it only needs to default to None and then the ternary is just more compact.

if cutscene_ids is None: cutscene_ids = [] {"vsCutsceneIds": ListContainer(cutscene_ids)}

is more compact and readable imo than

{"vsCutsceneIds": ListContainer(cutscene_ids) if cutscene_ids is not None else ListContainer()}

to be fair, you only need if cutscene_ids there. I also personally like inlining it, but maybe that's the JavaScript background talking, haha. if the former way is more pythonic, I'll just go with that

yes, please do. though if you're making a new wrapper class, you should also convert this function into a classmethod to create the wrapper and then have this function accept an instance of the wrapper class

duncathan · 2025-03-04T05:02:54Z

src/mercury_engine_data_structures/formats/brsa.py

+                "sCharclassGroupId": charclass_group,
+                "asItemsIds": ListContainer([""] * 9),
+                "vsCameraCollisionsIds": ListContainer(camera_ids) if camera_ids else ListContainer(),
+                "vsCutscenesIds": ListContainer(cutscene_ids) if cutscene_ids else ListContainer(),


what's with these ternaries?

these are here because of the mutable default arguments

duncathan · 2025-03-04T05:08:36Z

src/mercury_engine_data_structures/formats/brsa.py

+        param subarea_id: the name of the subarea
+        param preset_name: the name of the preset
+        param setup_id: the name of the setup the subarea is in"""
+        self.get_subarea_config(subarea_id, setup_id).asItemsIds[8] = preset_name


ok I appreciate your concern about it being out of scope of this PR to abolish the use of Container here, and that's fine, but these functions are a problem. at the very minimum I need you to manually write a wrapper class for subarea configs cuz these are just untenable

do you mean just asItemsIds or the whole config? I can see this in particular being made a lot simpler by it and I wouldn't mind doing so as a temporary solution. something like BMSAD?

it would be easiest to add a wrapper that gets returned by get_subarea_config(). otherwise you would have to deal with some really messy code to couple the asItemsIds wrapper to the internal container, which is way more work than just adding properties to the config wrapper.

something like the BMSLD pr: https://github.com/randovania/mercury-engine-data-structures/pull/258/files#diff-bf70997618c433f3cb8e9e854527f73c9a3a8327c932b8bd6101897ec7c55857R145

yeah, definitely seems easiest.

duncathan · 2025-03-04T05:09:14Z

src/mercury_engine_data_structures/formats/brsa.py

+        returns: the charclass group"""
+        return next(group for group in self.charclass_groups if group.sId == group_id)
+
+    def add_charclass_group(self, group_id: str, charclasses: list[str] = []) -> Container:


mutable default argument

duncathan · 2025-03-04T05:09:32Z

src/mercury_engine_data_structures/formats/brsa.py

+            raise ValueError(f"Charclass {group_id} is already present")
+
+        new_group = Container(
+            {"sId": group_id, "vsCharClassesIds": ListContainer(charclasses) if charclasses else ListContainer()}


another weird ternary

MayberryZoom and others added 2 commits March 1, 2025 23:26

Add more methods to BRSA

1b7c838

[pre-commit.ci] auto fixes from pre-commit.com hooks

e49ee6d

for more information, see https://pre-commit.ci

MayberryZoom added the game: Metroid Dread label Mar 2, 2025

duncathan requested changes Mar 2, 2025

View reviewed changes

MayberryZoom and others added 3 commits March 2, 2025 18:11

Use ListContainers when adding new elements to BRSA

d89443e

Don't allow existing setups/subareas/charclass groups to be added

311d7d0

[pre-commit.ci] auto fixes from pre-commit.com hooks

d50c4a6

for more information, see https://pre-commit.ci

Add missing type hints to add_subarea_config

e389c1a

MayberryZoom requested a review from duncathan March 3, 2025 01:59

duncathan requested changes Mar 4, 2025

View reviewed changes

Add more methods to BRSA #280

Are you sure you want to change the base?

Add more methods to BRSA #280

Uh oh!

Conversation

MayberryZoom commented Mar 2, 2025

Uh oh!

codecov bot commented Mar 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

duncathan left a comment

Choose a reason for hiding this comment

Uh oh!

MayberryZoom commented Mar 2, 2025

Uh oh!

MayberryZoom commented Mar 2, 2025

Uh oh!

MayberryZoom commented Mar 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Mar 2, 2025 •

edited

Loading