Keep entity property type names to be used by smart editors

[LogicAndTrick]

I have some code that's already implemented this change to some degree, but I think it deserves some discussion beforehand in case there's better possibilities.

Currently TB has "smart editors" for:

• flags (matches on the FlagsProperty definition type)
• choice (matches on the ChoiceProperty definition type)
• color (matches by name)
• wad (matches by name & classname)

What I would like (for now) is for the "color" smart editor to be a bit more flexible than just matching by name - for example, HL has a standard key called rendercolor on almost all brush entities. Instead of just including more hard-coded names in the smart editor, it would be great if the behaviour could instead be driven by the data types available in the entity definition format, if it's available.

FGDs also have multiple colour types: color1 and color255 to indicate RGB colours that should have components of either float (1 0 0.5) or byte (255 0 127). It would be great if the colour smart editor would have access to see if one of these two types are used and automatically select the correct colour formatting to use instead of relying on the user to know which option to select.

Additionally I think the current PropertyDefinitionType enum is serving a dual purpose that isn't currently used and could be replaced by a change like this. The enum values are currently:

enum class PropertyDefinitionType
{
  TargetSourceProperty,
  TargetDestinationProperty,
  StringProperty,
  BooleanProperty,
  IntegerProperty,
  FloatProperty,
  ChoiceProperty,
  FlagsProperty
};

If we ignore the first two types (and arguably, ChoiceProperty), they all seem to make sense - they represent the underlying data type that the value stores. But TargetSourceProperty and TargetDestinationProperty are outliers, in my opinion. Those two values are represented as strings internally so it makes sense they are StringProperties, much like the color properties are also represented as strings and don't have their own special ColorProperty entry in the enum.

My first thought was to simply add a ColorProperty type to this enum and update the code to match. However, this wouldn't allow the color1 and color255 types to make it to the smart editor, so you would need multiple colour properties, such as Color1Property, Color255Property, and then ColorProperty as a fallback type for entity definitions that don't specify the exact storage method. This starts to get quite shaky as you add more property types (FGD by itself has quite a few at this point, though you wouldn't need smart editors for the majority of them).

I think a better option would be to add a new field to the PropertyDefinition called typeName (or similar) which retains the original value of the property's type from the FGD/DEF file. The typeName can then be used by smart editors and potentially other editor features as a "hint" which informs us more specifically what the value's purpose is.

This would not only be useful for removing hard-coding of property names from the code (e.g. "color", "*_color", "*_color2", "*_colour"), but would also allow for easier future extension of new types and smart editors (e.g. a file browser for sprite/model/sound paths is very useful for HL editing and HL FGDs use sprite, studio, and sound property types for this purpose).

I also think a change like this would open up the possibilities of moving the smart editor configuration into the game config, rather than hard-coding it into the editor - much like how the "tags" functionality already works. This would make configuration of "generic" game types much more flexible, as it could be changed by the user without needing to recompile the code.

For example, perhaps one day HL could be configured like:

"entities": { ...,
  "editors": [{
    "match": "typename",
    "pattern": "studio",
    "editor": "filebrowser",
    "properties": {
      "filetype": "Models (*.mdl)",
      "filter": "*.mdl"
    }
  }, ...]
}

[kduske]

I would suggest to keep this simple and just extend the smart editor matcher for the color editor to cover the cases you want. We can of course extend the matchers to look at the property definitions, too. But remember that not all FGDs are well maintained, so we would prefer to keep everything working without having any property definitions. That's also the reason for the current design.

I have some code that's already implemented this change to some degree, but I think it deserves some discussion beforehand in case there's better possibilities.

Additionally I think the current PropertyDefinitionType enum is serving a dual purpose that isn't currently used and could be replaced by a change like this. The enum values are currently:

enum class PropertyDefinitionType
{
  TargetSourceProperty,
  TargetDestinationProperty,
  StringProperty,
  BooleanProperty,
  IntegerProperty,
  FloatProperty,
  ChoiceProperty,
  FlagsProperty
};

I think this enum can be mostly replaced by using dynamic_cast. This code is very old and back then I avoided dynamic type checks. It would be fine to modernize this.

The target related properties are indeed outliers. But this is a larger topic. Currently, the names of these properties (targetname and so on) are hardcoded and TB relies on these for updating its cache of entity links. EntityNodeBase contains four vectors of entity pointers (m_linkSources and so on). These are all entities to which an entity is linked via these properties. TB relies on these caches for drawing entity links for example. The caches are updated automatically whenever an entity is changed, and they use an index of all entity property keys and values that is stored in WorldNode::m_entityNodeIndex.

This whole process is pretty involved, and could arguably do with an overhaul. But to get back to your issue: If you want to make targetname etc. configurable via the FGD, you have to keep these caches working. One part of this puzzle is EntityPropertyConfig, which can store the actual names of the linking properties that we read from the FGD file. But then you still have to drop and rebuild all caches when the FGD is reloaded.

I think a better option would be to add a new field to the PropertyDefinition called typeName (or similar) which retains the original value of the property's type from the FGD/DEF file. The typeName can then be used by smart editors and potentially other editor features as a "hint" which informs us more specifically what the value's purpose is.

No, I wouldn't do this. Rather, I would prefer it if we have an independent and concrete model of the property definitions that TB supports (via classnames or an enum or anything that can be checked by the compiler), and map whatever we read from FGD files to this model. The reason is that TB supports various kinds of entity definition files (FGD, DEF, ENT), and in the future we might want to add more. If we leak info that is specific to a specific file format directly into the editor, we will have to support any special cases that come along with these string types names forever. That sounds like a maintenance nightmare to me.

I also think a change like this would open up the possibilities of moving the smart editor configuration into the game config, rather than hard-coding it into the editor - much like how the "tags" functionality already works. This would make configuration of "generic" game types much more flexible, as it could be changed by the user without needing to recompile the code.

I would not do this either, the game config files are already too complex, and we should rather try to use the entity definitions for this.

To summarize: You can fix your specific problem by extending the smart editor matcher to include any property that contains the word "color" or "colour". That sounds like a simple solution. But I would also be open to a solution that leverages info from the FGD files in addition to "just" the property names to improve the matcher. But this would require some refactoring of TB's entity property definitions to represent the property type we read from the FGD file. I'd be open to this and would like to help to design this, if you want to go this route.

[LogicAndTrick]

Oh, I didn't see the issue for FGD property types, that pretty much covers the issues I'm facing with the colour editor, so maybe it'd be better to close this out in favour of that one? I'm willing to help out the implementation once a good design can be worked out.

I get where you're coming from in regards to the extra maintenance added by leaking the definition through to the rest of the app. I didn't really consider that aspect. Changing the smart editor matcher would get 80% of the way there, but I'd really also like to be able to tell the difference between a color1 property and a color255 property in the smart editor. That would require at least some property type information to be kept and passed through to the smart editor. An enum would work for that.

There's also the slightly more complicated cases of studio, sound, and sprite which would all have a "file browser" smart editor, with different filters specified in the browser. What gets even more complicated is that for HL, these file browsers are rooted at different levels, for example:

• sprite: sprites/example.spr
• model: models/example.mdl
• sound: example.wav (the sounds/ prefix is added by the engine)

So if we wanted first-class support for these, we'd need to either hard-code this behaviour into the app, or alternatively add some optional syntax to TB FGDs for this to be configured (it should be noted that Source2 FGDs have some additional syntax that might be useful in this space). Either way doesn't sound very nice in my opinion, is there a better approach?

(I realise that file browser smart editors are not in scope for now, but if changes are being made in this area then it would be nice to try and cover this potential future functionality as well)