search

Basement-Renovator / basement-renovator

A room editor for Binding of Isaac Rebirth
Other
66 stars 29 forks source link

Entity XML lookup overhaul + Dynamic parameter support #262

Closed Meowlala closed 3 years ago

Meowlala commented 3 years ago

This replaces every instance where global entityXML was used with a new xmlLookups.entities class that can be used consistently. Now, rather than using entity XML nodes to get entity data, an EntityConfig class is used instead, which can precalculate certain values and store other new data.

This also adds a new system for adding parameters to objects that can be easily extended through xml. Here's an example, replicating the old Ball and Chain features through XML:

<entity BaseHP="100" Group="Spikes" ID="893" Image="resources/Entities/893.0.0 - Ball and Chain.png" Kind="Enemies" Name="Ball and Chain" Variant="0" Subtype="0" >
    <param BitCount="1" BitOffset="8" Display="Dropdown">
        <value Name="Clockwise"/>
        <value Name="Counter clockwise"/>
    </param>
    <param Prefix="Speed - " BitCount="4" BitOffset="4"/>
    <param Prefix="Distance - " BitCount="4" BitOffset="0" Tooltip="Grid-Distance between the source block and the spike"/>
</entity>

It supports "Dropdown", "Spinner", "Slider", and "Dial" as its displays, as well as various fluff for visual interpretation like a prefix and suffix to the label, altered visual base value, and interpreting as a range of seconds. Eventually there should be a method by which modders can supply their own scripts for more complex displays and values, but this should cover a wide variety of use cases to start.

This also enables filtering for entities with parameters, which I don't think was working before.

Meowlala commented 3 years ago

I moved the icon format fixing to the fixupLookups function in BasementRenovator.py, but in running it I noticed a couple things:

There is probably some further review needed as to how to fix png formatting.

Meowlala commented 3 years ago

Alright, I've adjusted the parameter system to match roughly your guidelines. There are a few things off, though:

Further review is definitely needed!

Meowlala commented 3 years ago
  • It changes pretty much every png file in the repository.
  • It does not fix this warning from libpng: libpng warning: iCCP: known incorrect sRGB profile

My guess has been that there's a few random UI icons in BR that are triggering this. Maybe those could be added to the logic?

Will check this out. Should there be an additional XML file that defines extra sprites that should be loaded and fixed? Or a hardcoded list?

  • Key Type="bitfield" is to establish a consistent syntax for attaching metadata/configuration to attributes, much in the way group, kind, and tooltip do. This type of convention is one of the few benefits of xml over json.

I'm still just not following, I think. To me, parameters / attributes / bitfields seem distinctly separate from Variants and Subtypes, just happening to use them for data storage. A subtype or variant will not always be one of those things.

  • The more ambiguous names would allow changing the underlying widget; for example, spinners are kind of awful from a UX perspective and when and if we move to a non-Qt implementation I'd like to swap them out for something nicer to use.

Fair, but I would like the names to at least resemble the resulting widget. "scalar" and "range" are far too vague.

  • Minimum and Maximum don't have a use case in the base game, but are very useful for mods that want to have a parameter directly correspond to an in-game value. I'd rather keep them if possible.

See previous comment. Maybe Maximum at least can see use in the second range case. Why not use Maximum and Minimum on collectibles or pickups? It would be a bit contrived, but they seem like a reasonable-ish use-case. Failing that, please make sure at least one mod is using this feature.

StageAPI does use Minimum and Maximum, for a PickupPrice parameter which can go from -5 to 99, the full range of the game's price options. For collectibles and pickups in BR, I foresee a few problems:

  • Bit offset depending on the order the displays are in is terrible for organization; I'm fine with including an automatic version as a feature, but Offset should be manually specifiable. For instance, the current order of the displays for both entities is exactly opposite the bit order.

The main reason not to allow manual specification is to ensure that there's no overlaps of values accidentally and that all bits are filled. More validation should be added if we're keeping manual offsets. Either way, I'd prefer BR itself to not use it for at least one bitfield.

I'm fine with adding additional validation. And could probably avoid using it for Ball and Chain without too much trouble, since it doesn't use anything too large and unwieldy.

How about Attribute? i.e. an Entity's Attribute represented as a Bitfield allows configuring 1 or more other Attributes. I don't really think of Subtype as a Parameter, but it's definitely an Attribute. Feel free to substitute another name

Unsure about this. I've become happier with BitfieldElement since writing that last review, as wordy and unhandy as it is, since it's fairly accurate to the implementation. I don't think that an Attribute on its own sounds like something that would inherently be a BitfieldElement.

budjmt commented 3 years ago

Will check this out. Should there be an additional XML file that defines extra sprites that should be loaded and fixed? Or a hardcoded list?

FixIconFormat still kind of exists in the space of "dev feature" so hardcoded list is fine.

I'm still just not following, I think. To me, parameters / attributes / bitfields seem distinctly separate from Variants and Subtypes, just happening to use them for data storage. A subtype or variant will not always be one of those things.

From my perspective, Variant and SubType are just attributes that contain some information. Most of the time this is identifying information, but when used as bitfields they can also used for storage of other attributes/information. If Isaac weren't Isaac and allowed extending the saved format with more fields, I could see more of an argument for them being special. As it happens though, I thing treating them generically makes sense. It's very much a non-issue though, this is more thinking about long-term roadmap.

StageAPI does use Minimum and Maximum, for a PickupPrice parameter which can go from -5 to 99, the full range of the game's price options. For collectibles and pickups in BR, I foresee a few problems:

  • Collectible subtypes and pickup subtypes should be extensible by mods
  • Collectibles and pickups should update visually as their variant / subtype is changed, which is not currently very well supported

Both good points! StageAPI's usage should be fine, enough people have it enabled to (hopefully) catch bugs.

Unsure about this. I've become happier with BitfieldElement since writing that last review, as wordy and unhandy as it is, since it's fairly accurate to the implementation. I don't think that an Attribute on its own sounds like something that would inherently be a BitfieldElement.

Fair enough. I have no strong preference as the nomenclature may end up changing again down the road.

Meowlala commented 3 years ago

Running the new fully functioning icon fixer and doing some debugging reveals that certain images are not able to be fixed, despite being run through fixImage().

Examples include:

Haven't found a clean way to get a complete list of all offenders yet, unfortunately.

Meowlala commented 3 years ago

Latest commit looks to be all of the new issues addressed, so far as I can see.