Basic support for FGD input/output

[MicroTransactionsMatterToo]

Hello. I've recently started working on game using Trenchbroom for map creation, and I figured I'd open an issue to ask about the feasibility of some sort of support for input/output properties in FGD files.

Currently Trenchbroom will fail to parse an FGD file if it contains an input or output property in an FGD file, which is understandable given it doesn't have any support for them. My initial thoughts were to leverage them to enhance Trenchbrooms current entity target visualisation (which only checks target and targetname), and store entity output connections in a __tb property for each entity, but I'm not sure if that'd cause issues or not, since I'm not super familiar with the .map format

I'm likely to implement some sort of support myself just to make working on maps a bit less error-prone, and I'll try and submit it as a pull request if people think it seems to be a sensible feature to add.

[SirYodaJedi]

Source's I/O system, while stored uniquely in the VMF, is stored as just regular KVs in the compiled entity lump; it absolutely could be tacked on the the MAP format for people using TrenchBroom for their own purposes. Basically, if a KV name in the MAP matches an "output" in the FGD, then it would be treated it as an output instead of a KV.

Unedited example directly from the entity lump of Half-Life 2 Deathmatch's Halls3:

{
"model" "*3"
"origin" "-642 -450 76.61"
"spawnflags" "1"
"StartDisabled" "0"
"classname" "trigger_once"
"OnStartTouch" "teleporter1,SetAnimation,open,0,-1"
"OnStartTouch" "teleportsoundin1,PlaySound,,0,-1"
}

Syntax is "output" "target,input,parameter,time_delay,fire_limit"; see the I/O and FGD VDC pages for specifics, including existing FGD syntax.

Mentioning #1159 to link the issues

[MicroTransactionsMatterToo]

Oh, great. I wasn't aware that there were already examples of .map files with IO entries. From my limited research it seemed it was introduced in Source, and Hammer 4.0, which used .vmf files as opposed to .map files. I also wasn't aware that keys can occur more than once in an entity, which makes things a lot easier

I'm just working my way through the FGD parsing code at the moment to figure out adding parsing support. Ideally there'd also be some kind of dedicated UI for adding connections and good support in the entity linking visualisation, but that'll be secondary.

[SirYodaJedi]

Oh, great. I wasn't aware that there were already examples of .map files with IO entries. From my limited research it seemed it was introduced in Source, and Hammer 4.0, which used .vmf files as opposed to .map files.

The entity lump of many idtech-derived BSP formats are basically MAP files without brushwork, Source 1 included. It's not an actual MAP file, but strip the lump header, and you have a plaintext file that map editors can read.

I also wasn't aware that keys can occur more than once in an entity, which makes things a lot easier

It's technically possible, but most games don't expect it, so it seems TrenchBroom currently skips duplicate keys (if I'm reading this section of code correctly).

[kduske]

This seems very specific to HL, so I'm not sure we should add any support except for parsing the FGD syntax.

[MicroTransactionsMatterToo]

It's technically possible, but most games don't expect it, so it seems TrenchBroom currently skips duplicate keys (if I'm reading this section of code correctly).

Unfortunate, but was also what I'd expected initially. Guess .MAP outputs could have the key suffixed with a number or something.

This seems very specific to HL, so I'm not sure we should add any support except for parsing the FGD syntax.

I had figured that full support would probably be a bit out of scope. I'm planning to implement it personally regardless, so I figured I'd start an issue to figure out what would be worth contributing back.

[magicaldave]

Just wanted to toss my two cents in here, input/output support would help Morrobroom a lot.

[MicroTransactionsMatterToo]

@kduske Would appreciate your input on this. I'm trying to decide how to implement this feature, and it seems to me there are two main options. The first is to add a property like m_ioDirection to PropertyDefinition. The other is to make a new PropertyDefinition subclass named something like IOPropertyDefinition. I'm not entirely sure which would be better, so I figured I'd check to see if you have a preference

[kduske]

I don't understand the feature well enough to give you proper design feedback, but generally speaking we should avoid new classes. If this member is only meaningful for some property definitions, that could be expressed using std::optional.

[MicroTransactionsMatterToo]

I don't understand the feature well enough to give you proper design feedback, but generally speaking we should avoid new classes. If this member is only meaningful for some property definitions, that could be expressed using std::optional.

Right. Have in the meantime implemented it as a new class, you can see the commit here, but if you don't like the look if it I'll change approach. It's a bit awkward cause it's basically just another property, but the syntax is different from all the other property types, and the IO arguments have their own set of types (void, integer, float, string, bool).

So far what I've written passes tests, so I'm hoping it'll be fairly easy to just drop in the new functionality without having to change much.

[kduske]

Given my earlier comment about only adding support for parsing these definitions, why do you need to add anything to the property definitions at all?

[kduske]

But that aside, the way to model this really depends on how the feature works. The other subclasses of PropertyDefinition all represent different kinds of properties. If these IO properties are like that, and not somehow orthogonal to the other definitions, a subclass may be better suited.

Although the fact that you now have to model the value types in a different way again makes me think that it doesn't really fit with the current model and that it could be better to refactor the current code first so that you can get rid of all the subclasses and represent the value types differently. Then you could reuse these type definitions for your new IO properties.

The current subclasses could and should alle be removed and represented with a single class PropertyDefinition and a variant that holds the possible value types.

[MicroTransactionsMatterToo]

Given my earlier comment about only adding support for parsing these definitions, why do you need to add anything to the property definitions at all?

I had figured that if we were parsing it, we might as well make the information available in some form. Even if we just silently ignore it, we'd still need to spend time parsing the text and validating syntax.

But that aside, the way to model this really depends on how the feature works. The other subclasses of PropertyDefinition all represent different kinds of properties. If these IO properties are like that, and not somehow orthogonal to the other definitions, a subclass may be better suited.

Although the fact that you now have to model the value types in a different way again makes me think that it doesn't really fit with the current model and that it could be better to refactor the current code first so that you can get rid of all the subclasses and represent the value types differently. Then you could reuse these type definitions for your new IO properties.

I might go down this road then. Good chance to get more familiar with the codebase anyways

The current subclasses could and should alle be removed and represented with a single class PropertyDefinition and a variant that holds the possible value types.

Just to make sure I'm understanding this correctly, by variant you mean std::variant right?

[kduske]

I had figured that if we were parsing it, we might as well make the information available in some form. Even if we just silently ignore it, we'd still need to spend time parsing the text and validating syntax.

The easiest thing would be to just parse it and drop the info since we are not using it. I prefer doing the easiest thing.

The current subclasses could and should alle be removed and represented with a single class PropertyDefinition and a variant that holds the possible value types.

Just to make sure I'm understanding this correctly, by variant you mean std::variant right?

Yes, exactly. The idea is to end up with once class PropertyDefinition that roughly looks like this:

template <typename T>
struct PropertyValueT {
  T value;
  std::optional<T> defaultValue;
};

using PropertyValue = std::variant<PropertyValueT<std::string>, PropertyValueT<float>, ...>;

class PropertyDefinition {
  ...
private:
  PropertyValue m_value;
};

With this design we can fold the optional default value into PropertyValue and make it type safe so that the default value must have the same type as the value itself.

[kduske]

I have probably omitted some details that might affect the design, but this should be the gist of it.

[MicroTransactionsMatterToo]

Yes, exactly. The idea is to end up with once class PropertyDefinition that roughly looks like this:

template <typename T>
struct PropertyValueT {
  T value;
  std::optional<T> defaultValue;
};

using PropertyValue = std::variant<PropertyValueT<std::string>, PropertyValueT<float>, ...>;

class PropertyDefinition {
  ...
private:
  PropertyValue m_value;
};

While this solution is nice, it doesn't really make sense for the PropertyDefinition class. A property definition doesn't really have a value in any meaningful sense except for it's default value. Outside of flags, choices and possibly input/output definitions in an FGD file, which arguably do have a value. Yesterday I did a rewrite of the class to reduce it into a single class, and simplify the parsing code, which you can see here.

It's probably not the most elegant solution, but it seems like it'll work well enough, and should be easy to extend in future if needed. At this point I just need to refactor the places where PropertyDefinitions are used.

[kduske]

While this solution is nice, it doesn't really make sense for the PropertyDefinition class. A property definition doesn't really have a value in any meaningful sense except for it's default value.

Yeah, that's right. I haven't worked with this code in a long time. Either way, it would be important to somehow enforce that the types of the default value and the property type match.

Outside of flags, choices and possibly input/output definitions in an FGD file, which arguably do have a value. Yesterday I did a rewrite of the class to reduce it into a single class, and simplify the parsing code, which you can see here.

It's probably not the most elegant solution, but it seems like it'll work well enough, and should be easy to extend in future if needed. At this point I just need to refactor the places where PropertyDefinitions are used.

I haven't looked at it too closely, but I don't understand the design choices that I saw. Now, PropertyDefinition can have a type that's either a PropertyDefinitionType or an IOType. I don't know if this represents what can be modeled in the FGD file, but it seems odd to me. And now you can mix ChoiceOption and FlagOption - and monostate? That can't be right.

[MicroTransactionsMatterToo]

I haven't looked at it too closely, but I don't understand the design choices that I saw. Now, PropertyDefinition can have a type that's either a PropertyDefinitionType or an IOType. I don't know if this represents what can be modeled in the FGD file, but it seems odd to me. And now you can mix ChoiceOption and FlagOption - and monostate? That can't be right.

The intent was that the type property could be checked to determine both what kind of type it is, and whether it's IO or not. IO properties have their own set of types that are similar, but slightly different to everything else (i.e. for most properties it's string, integer, float, boolean, choices, flags, but for IO it's void, integer, float, string, bool.)

As for mixing ChoiceOption FlagOption and monostate it'd probably be more sensible to make it optional<variant<ChoiceOption, FlagOption>>, likewise for default values.

Maybe it'd make more sense to make PropertyDefinition a templated class? I'm not familiar enough with C++'s templating to know if there's a sensible way to map that over though. I was thinking something that allows for a syntax like PropertyDefinition<PropertyType::String> or PropertyDefinition<PropertyType::Choices> or similar, where the typename given would determine the type of m_defaultValue and value() e.t.c., because a lot of the property types use strings for default values, and I'm not sure how to couple the properties type to the type of it's default value at compile time.

It's my first time working in C++ and making use of it's features properly, I normally use plain C, so apologies if I miss things that are obvious. Thanks for taking the time to provide feedback, it's much appreciated.