Refactor BRFLD

[MayberryZoom]

• Renames layer arguments to sublayer
• Adds argument to select layer
• Renames some methods to reflect this
• Makes argument naming convention consistent

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.33%. Comparing base (b4e1076) to head (7f3b6f7).
Report is 35 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #217      +/-   ##
==========================================
+ Coverage   75.54%   76.33%   +0.79%     
==========================================
  Files          78       78              
  Lines        3766     3782      +16     
==========================================
+ Hits         2845     2887      +42     
+ Misses        921      895      -26     

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

[duncathan]

some immediate thoughts: there should be an enum for layer, since it's always exactly the same three options. also, maybe the layer name should be kwarg-only? as-is the order of the args is kinda confusing. could also just swap em and break all the signatures I suppose

[MayberryZoom]

also, maybe the layer name should be kwarg-only? as-is the order of the args is kinda confusing. could also just swap em and break all the signatures I suppose

I'm not sure about this. In my head it makes the most logical sense to have the actor layer argument first, but most of the time you're going to be working with the entities layer specifically, so you'll only be using sublayer. If sublayer is positional and actor layer is keyword, is there really a point in making it keyword only? Or did you mean make both sublayer and actor layer kwarg only?

I honestly think it's fine as is. It's a bit funky but it should make sense when you're actually using the functions. And in practice it'll be easiest to work with, with the bonus of not breaking all the signatures.

[steven11sjf]

Also update tests/formats/test_brfld.py to fix the codecov stuff

[henriquegemignani]

I'll second the tests request

[MayberryZoom]

I added a new method for adding an actor to actor groups that doesn't assume the prefix eg_, since other actor layers use a different prefix (ssg_ for rSoundsLayer and lg_ for rLightsLayer). For now I made the existing add_actor_to_entity_groups use the old signature and had it just call the new add_actor_to_actor_groups with the prefix added, but if we're okay with breaking things, I'd rather have only the new method, personally.

[henriquegemignani]

Seems fine overall, but I'll leave for @duncathan to do a final say.

Things that would be neat, especially since we're polishing this up:

• docstrings for the changed methods
• tests for the other methods as well, such as remove_actor_from_group

[henriquegemignani]

Ah except for the part where the tests are failing.

[MayberryZoom]

Yeah, I just saw that there's a test failing. I'll fix it soon and see if I can get on some of those other improvements.

[steven11sjf]

I added a new method for adding an actor to actor groups that doesn't assume the prefix eg_, since other actor layers use a different prefix (ssg_ for rSoundsLayer and lg_ for rLightsLayer). For now I made the existing add_actor_to_entity_groups use the old signature and had it just call the new add_actor_to_actor_groups with the prefix added, but if we're okay with breaking things, I'd rather have only the new method, personally.

You could add a value to the enum for cc_prefix (make the value ENTITIES = "rEntitiesLayer", "eg_", and override with this in ActorLayer:

@classmethod
def __new__(cls, value: str, prefix: str):
    member = super.__new__(cls, value)
    member.cc_prefix = prefix
    return member

(might wanna cc @henriquegemignani / @duncathan to check if that should be new or init, i'm not sure if it's "pythonic" or whatever lmao)

[MayberryZoom]

I added a new method for adding an actor to actor groups that doesn't assume the prefix eg_, since other actor layers use a different prefix (ssg_ for rSoundsLayer and lg_ for rLightsLayer). For now I made the existing add_actor_to_entity_groups use the old signature and had it just call the new add_actor_to_actor_groups with the prefix added, but if we're okay with breaking things, I'd rather have only the new method, personally.

You could add a value to the enum for cc_prefix (make the value ENTITIES = "rEntitiesLayer", "eg_", and override with this in ActorLayer

Honestly, you should probably just be using the prefix when you call the method anyways. Assuming it is really only marginally more readable, while removing some of the library's power. If those 3-4 characters are really too much for someone utilizing this library, they should just subclass or write their own function to do it IMO.