randovania / mercury-engine-data-structures

Construct type definitions for Mercury Engine.
MIT License
2 stars 11 forks source link

Refactor BRFLD #217

Closed MayberryZoom closed 1 month ago

MayberryZoom commented 3 months ago
codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

MayberryZoom commented 1 month ago

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.

MayberryZoom commented 1 month ago

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 commented 1 month ago

Ah except for the part where the tests are failing.

MayberryZoom commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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.