QodotPlugin / qodot-plugin

(LEGACY) Quake .map support for Godot 3.x
MIT License
960 stars 70 forks source link

[WIP] Add entity properties #69

Closed rand0m-cloud closed 4 years ago

rand0m-cloud commented 4 years ago

Sorry for intruding on #66, this is better suited as a PR to be considered after #66 is merged. This work is based off of #66's last commit.

I added a new resource called QodotEntityProperties that allows the user to define the property's name, short/long description, and a default value. The resource is exported through the QodotPointEntityDefinition.

I modified the build_entity_spawns to check for defined properties and apply any defined properties found to the constructed node. It is expected that the user defines variables on their entity's scene and creates a property definition matching it.

The current work only includes integer and string property defining, and needs to be expanded to the available property types. I am unsure how to implement flags or choice types, so I am asking for discussion around it.

Finally, this work has some bugs that I am not sure how to solve. If I do:

  1. Define an entity with a custom property
  2. Create the entity in TrenchBroom with the property set to a value
  3. Duplicate the entity
  4. Reload the map into Qodot

Only one of the instanced entities has the custom value instead of both entities having the custom value.

Shfty commented 4 years ago

Apologies for the slow response. I've pulled down the changes and given them a look over, and I think this is handy functionality to have. However, it's not mergeable as-is. There's an issue with the FGD generation, and I'm not sure about the structure- having one resource file per property seems like overkill.

Instead, I'd expose a Dictionary inside the entity definition itself and take advantage of the inspector UI's ability to define types for each entry. That way, if dictionary['property'] is string: and the like could be used to determine the format during export.

Flags is a tricky one, since it's ultimately exposed as a bitmask. I'd implement that as a Dictionary of bools- that way each flag can have a name stored in its key, and the attached booleans can be used to determine defaults (if default flags are supported by the FGD format, i'm not sure offhand).

I'd probably implement choices as a Dictionary as well, since that allows a key and value to be specified for each choice. It does introduce some complexity in terms of identifying whether a dictionary contains flags or choices- that might be solvable by wrapping each one in its own custom class, but it might become cumbersome depending on that state of inspector property exports in 3.2.

Going back to the issue with FGD generation, it looks like default values for string properties are broken. The generated FGD doesn't have double-quotes around them, which causes TrenchBroom to error out during parse. Adding them manually makes it work as expected.

Shfty commented 4 years ago

I went ahead and refactored the existing FGD export support to include BaseClass and SolidClass as well as PointClass, and setup more flexible systems for defining class metaproperties, properties, descriptions, and so on.

The implementation is largely as mentioned in my previous comment, though I ended up separating choices and flags into dictionaries and nested arrays respectively since there's no custom class support in the dictionary inspector. Flags are a tad clunky, but they work and get all the data across as needed.

Next up is hooking up a flexible map file > entity workflow, which this pull partially covers. I'll take a look into this over the weekend.

rand0m-cloud commented 4 years ago

I've taken a look at the latest commits and I like your implementation over mine. You can close the pull request unless you want further discussion here.

Shfty commented 4 years ago

Alrighty, thanks for the contribution- sometimes it helps to have someone poke me with some code in order to get a new feature off the ground!

Further discussion can go on in #8