Basement-Renovator / basement-renovator

A room editor for Binding of Isaac Rebirth
Other
64 stars 28 forks source link

Entity grouping overhaul + tags system #267

Closed Meowlala closed 2 years ago

Meowlala commented 3 years ago

This replaces the Kind and Group systems in Basement Renovator as well as all associated hardcoding with a new unified Group system, allowing easily defining new tabs and the ordering of said tabs.

Currently I've only done minor work on the xml itself, but this should function as a proof of concept.

Some examples of the new <group> tag in action:

<!-- It's possible to create a group without a label or tab, useful for transcluding within other groups -->
<group Name="Random Collectibles">
  <!-- Entity definitions can be made within groups. 
  This is WIP, it should also be possible to reference an entity without redefining it -->
  <entity ID="5" Variant="100" Subtype="0" Name="Random Collectible" Image="resources/Entities/Items/5.100.0 - Random.png"/>
  <entity ID="5" Variant="150" Subtype="0" Name="Random Shop Item" Image="resources/Entities/Items/5.150.0 - Random.png"/>
  <entity ID="5" Variant="350" Subtype="0" Name="Random Trinket" Image="resources/Entities/5.350.0 - Trinket.png"/>
</group>

<!-- The IsTab and optional IconSize attributes can be used to denote that a group should have its own tab, and its properties  -->
<group Name="Collect" IsTab="1" IconSize="32, 64">
  <!-- Order in XML directly determines order in-editor, without any other sorting. Groups must be named uniquely. 
  Labels are optional, but if defined will enable a group header showing up wherever the group is included.
  You can also define a Label without a Name, in which case the Name automatically becomes the Label. -->
  <group Name="(Collect) Random" Label="Random">
    <!-- References to a previously defined group transclude all of that group's current and future contents into this one 
    Groups can be nested indefinitely. -->
    <group Name="Random Collectibles"/>
  </group>
</group>
Meowlala commented 3 years ago

Now that Antibirth is back to being supported again and a lot of refactoring has been done, it's pretty much down to the organizational elements.

The one big thing remaining that comes to mind is EntitiesRepentanceEmpty.xml, which will need to be rethought entirely; a simple drag and drop replacement just isn't feasible now that the files are split, and I don't know what its purpose is exactly. Whatever it is used for should probably become another entity tag.

Speaking of entity tags, I'm considering adding a new tag system for entities, like so:

<entity ...>
  <tag>Grid</tag>
  <tag>InEmptyRooms</tag>
</entity>

Tags would be searchable via the lookup functions and used in place of IsGrid and InEmptyRooms currently. This would be useful for new tags such as a special version of the InEmptyRooms tag, and would also allow for entities that relate to certain other entities in the future, like how Ball and Chains connect to certain grid entities in the room.

There are some hangups with it, though. Primarily, it'd make grid entity and empty room entity definitions much larger than they currently are, if backwards compatibility was not supported. Backwards compatibility will be easy enough, but maybe it'd be a good idea to have an alternative CSV Tag="Tag1,Tag2,Tag3" attribute in XML for less lines?

Other tags that could potentially be wrapped into this system include "DisableOffsetIndicator", "NoBlockDoors", "UsePitTiling", and "UseRockTiling",

Zamiell commented 3 years ago

Tags would be searchable via the lookup functions and used in place of IsGrid and InEmptyRooms currently.

are they not currently searchable? what do you mean by "searchable" exactly?

Meowlala commented 3 years ago

Tags would be searchable via the lookup functions and used in place of IsGrid and InEmptyRooms currently.

are they not currently searchable? what do you mean by "searchable" exactly?

I'm referring to tags that are searchable by Basement Renovator itself; currently, lookup only allows checking for entitytype, variant, subtype, inEmptyRooms, and name. With a new tags system, inEmptyRooms would be replaced by tags=[], a list of tags that must all be matched in order to match an entity.

Tags could also potentially get their own room search widget to search for rooms containing entities with any tag, but this might be a bit inconvenient if there are too many insignificant / "meta" tags.

Zamiell commented 3 years ago

it seems weird not to use native XML tags for tags. can we just refactor:

        def matches(self, entitytype=None, variant=None, subtype=None):
            if entitytype is not None and self.type != entitytype:
                return False
            if variant is not None and (
                self.variant != variant and not self.hasBitfieldKey("Variant")
            ):
                return False
            if subtype is not None and (
                self.subtype != subtype and not self.hasBitfieldKey("Subtype")
            ):
                return False

            return True

to something like

        def matches(self, filterObject=None):
            if filterObject is None:
              return true

           for field in filterObject:
               if field not in self:
                 return false
               if self.field != field:
                 return false

            return true
Meowlala commented 3 years ago

The lookup system isn't related to the native XML data at all, EntityConfig abstracts all XML-related information away.

Your version of matches would support detecting IsGrid and InEmptyRooms entities consistently, but it would not be at all automatically extendible to other tags, since each tag would still need to be mapped via EntityConfig beforehand. It also completely ignores the hasBitfieldKey checks, which are important since Subtype and Variant should be ignored when searching for entities with parameters using them.

To me, it makes far more sense for tags to get their own system, rather than a pile of attributes thrown directly into EntityConfig for each odd thing.

Zamiell commented 3 years ago

in a custom tag system, would it automatically code the filter UI? this feature only strikes me as useful if the end-user can add a new tag to an XML file, and have everything "just work" in BR automatically

otherwise, i think there is a benefit to having a BasementRenovatorEntity class that is strongly typed and defines every legal tag. such a data structure is good because it prevents typos and garbage data

Meowlala commented 3 years ago

Yes. The end user would be able to create new filterable tags, which could be added automatically to the "Room Filter Configuration" UI.

Preferably, users would be able to designate which tags they want to be filterable, perhaps along the lines of: <tag Filterable="1">InEmptyRooms</tag>

The problem with pre-defining every legal tag is that some entities will benefit from relational tags. An example of this is the Ball and Chain, which can connect to certain other entities in the room, mostly rocks. With tags, that could be done simply by tagging each grid entity that Ball and Chains can connect to with a BallAndChainConnectable tag, then adding some SnapToTag="BallAndChainConnectable" display property to the Ball and Chain definition.

Zamiell commented 3 years ago

that system sounds good to me. shouldn't we make all tags filterable? otherwise, if you want to have metadata about tags, then define all of them in a tags.xml file, and have BR throw an error if it reads an entity with a non-defined tag

Meowlala commented 3 years ago

Most tags other than InEmptyRooms (and the alternate stricter InEmptyRooms tag replacing EntitiesRepentanceEmpty.xml) wouldn't actually be very helpful to search. How often do you want to find rooms that contain / don't contain grid entities? Or on the extreme end, why would anyone ever want to filter for entities with the "DisableOffsetIndicator" tag?

A Tags.xml file sounds reasonable, would be small but effective for the purpose.

Zamiell commented 3 years ago

looks like you need to rebase on master

Meowlala commented 3 years ago

This branch is up to date with main, I think it's more of an issue on @epfly6's end?

Zamiell commented 3 years ago

your pr includes a change to ci.yml, which appears to be a bug, is that intentional?

Meowlala commented 2 years ago

With that last commit I think that's every review note addressed, let me know what you think!