Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
172 changes: 167 additions & 5 deletions src/mercury_engine_data_structures/formats/brsa.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,15 @@
import functools
from typing import TYPE_CHECKING

from construct import Container, ListContainer

from mercury_engine_data_structures.base_resource import BaseResource
from mercury_engine_data_structures.formats import standard_format

if TYPE_CHECKING:
from collections.abc import Iterator

from construct import Construct, Container
from construct import Construct

Check warning on line 14 in src/mercury_engine_data_structures/formats/brsa.py

View check run for this annotation

Codecov / codecov/patch

src/mercury_engine_data_structures/formats/brsa.py#L14

Added line #L14 was not covered by tests

from mercury_engine_data_structures.game_check import Game

Expand All @@ -24,8 +26,168 @@
def subarea_setups(self) -> Iterator[Container]:
yield from self.raw.Root.pSubareaManager.vSubareaSetups

def get_subarea_setup(self, id: str) -> Container:
return next(setup for setup in self.subarea_setups if setup.sId == id)
@property
def charclass_groups(self) -> Iterator[Container]:
yield from self.raw.Root.pSubareaManager.vCharclassGroups

def get_subarea_setup(self, setup_id: str) -> Container:
"""Gets a setup
param setup_id: the name of the setup
returns: the setup"""
return next(setup for setup in self.subarea_setups if setup.sId == setup_id)

def add_setup(self, setup_id: str) -> Container:
"""Adds a new setup
param setup_id: the name of the new setup
returns: the newly created setup"""
if next((setup for setup in self.subarea_setups if setup.sId == setup_id), None):
raise ValueError(f"Setup {setup_id} is already present")

new_setup = Container({"sId": setup_id, "vSubareaConfigs": ListContainer()})
self.raw.Root.pSubareaManager.vSubareaSetups.append(new_setup)

return new_setup

def get_subarea_config(self, subarea_id: str, setup_id: str = "Default") -> Container:
"""Gets a config for a subarea
param subarea_id: the name of the subarea the config is for
param setup_id: the name of the setup the config is in
returns: the config for the subarea"""
return next(config for config in self.get_subarea_setup(setup_id).vSubareaConfigs if config.sId == subarea_id)

def add_subarea_config(
self,
subarea_id: str,
setup_id: str = "Default",
*,
disable_subarea: bool = False,
camera_distance: float = -1.0,
ignore_camera_offsets: bool = False,
charclass_group: str = "No Enemies",
camera_ids: list[str] = [],
cutscene_ids: list[str] = [],
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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()}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

) -> Container:
"""Adds a config for a subarea
param subarea_id: the name of the subarea the config is for
param setup_id: the name of the setup the config will be in
returns: the newly created config"""
if next(
(config for config in self.get_subarea_setup(setup_id).vSubareaConfigs if config.sId == subarea_id), None
):
raise ValueError(f"Config for {subarea_id} is already present in {setup_id}")

new_config = Container(
{
"sId": subarea_id,
"sSetupId": setup_id,
"bDisableSubarea": disable_subarea,
"fCameraZDistance": camera_distance,
"bIgnoreMetroidCameraOffsets": ignore_camera_offsets,
"sCharclassGroupId": charclass_group,
"asItemsIds": ListContainer([""] * 9),
"vsCameraCollisionsIds": ListContainer(camera_ids) if camera_ids else ListContainer(),
"vsCutscenesIds": ListContainer(cutscene_ids) if cutscene_ids else ListContainer(),
Copy link
Contributor

Choose a reason for hiding this comment

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

what's with these ternaries?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

these are here because of the mutable default arguments

}
)
self.get_subarea_setup(setup_id).vSubareaConfigs.append(new_config)

return new_config

def set_scenario_collider(self, subarea_id: str, collider_name: str, setup_id: str = "Default") -> None:
"""Sets the scenario collider for a subarea
param subarea_id: the name of the subarea
param collider_name: the name of the collider
param setup_id: the name of the setup the subarea is in"""
self.get_subarea_config(subarea_id, setup_id).asItemsIds[0] = collider_name

def set_light_group(self, subarea_id: str, group_name: str, setup_id: str = "Default") -> None:
"""Sets the light group for a subarea
param subarea_id: the name of the subarea
param group_name: the name of the group
param setup_id: the name of the setup the subarea is in"""
self.get_subarea_config(subarea_id, setup_id).asItemsIds[1] = group_name

def set_sound_group(self, subarea_id: str, group_name: str, setup_id: str = "Default") -> None:
"""Sets the sound group for a subarea
param subarea_id: the name of the subarea
param collider_name: the name of the group
param setup_id: the name of the setup the subarea is in"""
self.get_subarea_config(subarea_id, setup_id).asItemsIds[2] = group_name

def set_scene_group(self, subarea_id: str, group_name: str, setup_id: str = "Default") -> None:
"""Sets the scene group for a subarea
param subarea_id: the name of the subarea
param group_name: the name of the group
param setup_id: the name of the setup the subarea is in"""
self.get_subarea_config(subarea_id, setup_id).asItemsIds[3] = group_name

def set_entity_group(self, subarea_id: str, group_name: str, setup_id: str = "Default") -> None:
"""Sets the entity group for a subarea
param subarea_id: the name of the subarea
param group_name: the name of the group
param setup_id: the name of the setup the subarea is in"""
self.get_subarea_config(subarea_id, setup_id).asItemsIds[4] = group_name

def set_tilegroup_group(self, subarea_id: str, group_name: str, setup_id: str = "Default") -> None:
"""Sets the tilegroup group for a subarea
param subarea_id: the name of the subarea
param group_name: the name of the group
param setup_id: the name of the setup the subarea is in"""
self.get_subarea_config(subarea_id, setup_id).asItemsIds[5] = group_name

def set_visual_preset(self, subarea_id: str, preset_name: str, setup_id: str = "Default") -> None:
"""Sets the visual preset for a subarea
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[6] = preset_name

def set_sound_preset(self, subarea_id: str, preset_name: str, setup_id: str = "Default") -> None:
"""Sets the sound preset for a subarea
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[7] = preset_name

def set_music_preset(self, subarea_id: str, preset_name: str, setup_id: str = "Default") -> None:
"""Sets the music preset for a subarea
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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, definitely seems easiest.


def get_charclass_group(self, group_id: str) -> Container:
"""Gets a charclass group
param group_id: the name of the group
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:
Copy link
Contributor

Choose a reason for hiding this comment

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

mutable default argument

"""Adds a new charclass group
param group_id: the name of the new group
param charclasses: the charclasses in this group
returns: the newly created charclass group"""
if next((group for group in self.charclass_groups if group.sId == group_id), None):
raise ValueError(f"Charclass {group_id} is already present")

new_group = Container(
{"sId": group_id, "vsCharClassesIds": ListContainer(charclasses) if charclasses else ListContainer()}
Copy link
Contributor

Choose a reason for hiding this comment

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

another weird ternary

)
self.raw.Root.pSubareaManager.vCharclassGroups.append(new_group)

def get_subarea_config(self, id: str, setup_id: str) -> Container:
return next(config for config in self.get_subarea_setup(setup_id).vSubareaConfigs if config.sId == id)
return new_group
55 changes: 55 additions & 0 deletions tests/formats/test_brsa.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,58 @@ def test_dread_brsa_100(dread_tree_100, brsa_path):
@pytest.mark.parametrize("brsa_path", bossrush_assets)
def test_dread_brsa_210(dread_tree_210, brsa_path):
parse_build_compare_editor(Brsa, dread_tree_210, brsa_path)


def test_add_setup(dread_tree_100):
subareas = dread_tree_100.get_file("maps/levels/c10_samus/s010_cave/s010_cave.brsa", Brsa)
subareas.add_setup("Test")

assert len(list(subareas.subarea_setups)) == 18

with pytest.raises(ValueError, match=r"Setup .+? is already present"):
subareas.add_setup("Default")


def test_add_subarea_config(dread_tree_100):
subareas = dread_tree_100.get_file("maps/levels/c10_samus/s010_cave/s010_cave.brsa", Brsa)
new_config = subareas.add_subarea_config("camera_test")

assert len(subareas.get_subarea_setup("Default").vSubareaConfigs) == 79

with pytest.raises(ValueError, match=r"Config for .+? is already present in .+?"):
subareas.add_subarea_config("collision_camera_000")

subareas.set_scenario_collider("camera_test", "collider_test")
subareas.set_light_group("camera_test", "lg_test")
subareas.set_sound_group("camera_test", "ssg_test")
subareas.set_scene_group("camera_test", "sg_test")
subareas.set_entity_group("camera_test", "eg_test")
subareas.set_tilegroup_group("camera_test", "bg_test")
subareas.set_visual_preset("camera_test", "visual_test")
subareas.set_sound_preset("camera_test", "sound_test")
subareas.set_music_preset("camera_test", "music_test")

for index, to_check in enumerate(
[
"collider_test",
"lg_test",
"ssg_test",
"sg_test",
"eg_test",
"bg_test",
"visual_test",
"sound_test",
"music_test",
]
):
assert new_config.asItemsIds[index] == to_check


def test_charclasses(dread_tree_100):
subareas = dread_tree_100.get_file("maps/levels/c10_samus/s010_cave/s010_cave.brsa", Brsa)
subareas.add_charclass_group("Test", ["klaida"])

assert len(subareas.get_charclass_group("Test").vsCharClassesIds) == 1

with pytest.raises(ValueError, match=r"Charclass .+? is already present"):
subareas.add_charclass_group("No Enemies")