Fix base classes and default values

[deadhostdave]

During the 'fetch_entity_definitions' build step, the qodot-plugin is only considering 1 level of base class hierarchy when merging properties into the entity. I believe FGD supports a multi-level hierarchy, so shouldn't all base classes be traversed while merging in properties?

In addition, when the 'apply_properties' build step is executed, only existing properties are evaluated - and properties defined in the entity definition are ignored. Unassigned properties - at least in TrenchBroom - are assumed to be 'default' value. Therefore, shouldn't missing/unassigned properties be set to the defaults defined in the entity definition?

[Shfty]

Non-recursive base class evaluation is an oversight; I used the Quake 1 FGD to model Qodot's entity pipeline, which iirc only has basic single-level inheritance.

This is a good feature to have, but I'm concerned about circular references - from the look of it, GDScript will go into an infinite loop on map build if any of the classes create a self-referential inheritance chain (ex. A -> B -> C -> A). It'd be preferable to check whether a given base class has already been encountered, and early-out with a printed error if so.

Property defaults I'm less certain about, so would probably be better off as a separate PR if anything.

Your reasoning makes sense from a workflow standpoint, but TrenchBroom isn't authoritative over defaults in Quake - if unset, their values aren't written to the map file, and are left to the game implementation. In effect, FGD defaults exist as a QoL measure to give mappers context on what a value is likely to be if unset, but provide no guarantees if that value has changed engine-side.

Ideally Qodot's FGD pipeline should detect the values set in the associated script files rather than providing a hand-set default, but as far as I'm aware that data isn't accessible (through, say, ClassDB) so you'd have to instantiate the script in order to read it, which is a no-go considering the possible side effects.

[deadhostdave]

Excellent point on the circular inheritance issue. I should've thought to add a check skip-or-error to avoid recursion if base_class already in the base_classes array.

Edit: Interestingly, Resouce files are protected from circular references. When trying to test a fix, the FGD tres reported scene/resources/resource_format_text.cpp:1392 - Circular reference to resource being saved found: 'res://game_definitions/fgd/base_classes/targetname.tres' will be null next time it's loaded - but I suppose an explicit check for defence-in-depth would be best. TrenchBroom also reports Entity definition class hierarchy contains a cycle in this case.

For property defaults, I take your point that TB isn't authoritative, and other tools that deal with this differently.

Edit: Removed unneeded comments.

[Shfty]

Thanks, the check looks good. For the error itself, I think it would be better done via printerr than push_error since the latter leans more toward runtime debugging semantics, and isn't consistent with built-in editor-level messages like Resource file not found.

On the defaults stuff, don't worry about commenting on how things are implemented - I'm open to being challenged there if there's a strong argument to be made for a given feature 🙂

In this case, I think I have some mental crosstalk going on with the qodot-next codebase: There are more options for handling property application under the new model that have the 'game is authoritative' approach make more sense. Namely, one that applies properties directly instead of using a hardcoded dictionary as a go-between, meaning defaults are encoded as export(String) var my_string := "default_value" much like they would be in Quake's C equivalent.

That's what I'm referring to when I say there's no way (that I know of) to retrieve property defaults from ClassDB - even with well-defined, well-typed properties, I wasn't able to find a way to extract that data in such a way that would allow the FGD export to automate things based on a reference to entity script, instead of requiring defaults to be manually entered.

[deadhostdave]

Good point - I've made that change.

For the defaults; I see where you are coming from. In my initial project, as I didn't want to have to keep the default values in the GDscript of a Qodot base/solid/point class in sync with the defaults I'd set in the Qodot TRES associated with that class (especially given the way TB was working as part of my workflow) I ended up preloading into a var the TRES file and prepopulating my 'properties' with its values, then overwriting/updating with new values when Qodot passed the subset of properties. At this point, I thought it would be better if instead of doing that, Qodot could just pass me the properties with the defaults already set... which is what I ended up with within the commit. However, I see that this might not be a one-size-fits-all solution.

I certainly like the 'game is authoritative' approach. Think I need to go check out qodot-next to understand it better! 😀