func-godot / func_godot_plugin

Quake .map support for Godot 4.2
MIT License
271 stars 18 forks source link

property created from TB lets entity not being creating on map build #28

Open azur-wolve opened 1 month ago

azur-wolve commented 1 month ago

After building a map that has an entity with a property created from TrenchBroom the map throws the following error line and such entity isnt built.

image

iirc the docs say that also properties created from TrenchBroom get added to the entity_properties within _func_godot_apply_properties() and to func_godot_properties dicts.

NOTE: the FuncGodotFGDEntityClass.class_properties hasn't been changed for that.


EDITED: have checked and seems to be that when creating a property on FuncGodotFGDEntityClass.class_properties whose string name has a space doenst throw any problem. But when the string name of a property created from TB has a space, it throws the error.

RhapsodyInGeek commented 1 month ago

I'm not getting the error, but I am missing the custom property if a space is added to the property name. I am getting the custom property without spaces in the property name.

It makes sense that it would add a property if it exists within the definition because that's how property defaults are handled. I haven't tested it yet, but I'm guessing that if you put in a custom value with the Class Properties property with spaces it'll use the default rather than the custom value.

Not sure why the parser skips it yet.

RhapsodyInGeek commented 1 month ago

Looking into it further, I believe I've found why it behaves that way.

There's this bit in the Map Parser Core, starting at Line 63:

for line in lines:
        if comment:
            comment = false
        var tokens := split_string(line, [" ", "\t"], true)
        for s in tokens:
            token(s)

So basically it splits up the tokens by spaces. In the token() function there's this part at Line 139:

FuncGodotMapParser.ParseScope.ENTITY:
            if buf_str.begins_with('"'):
                prop_key = buf_str.substr(1)
                if prop_key.ends_with('"'):
                    prop_key = prop_key.left(-1)
                    set_scope(FuncGodotMapParser.ParseScope.PROPERTY_VALUE)
            elif buf_str == "{":
                brush_idx += 1
                face_idx = -1
                set_scope(FuncGodotMapParser.ParseScope.BRUSH)
            elif buf_str == "}":
                commit_entity()
                set_scope(FuncGodotMapParser.ParseScope.FILE)
        FuncGodotMapParser.ParseScope.PROPERTY_VALUE:
            var is_first = buf_str[0] == '"'
            var is_last = buf_str.right(1) == '"'

            if is_first:
                if current_property != "":
                    current_property = ""

            if not is_last:
                current_property += buf_str + " "
            else:
                current_property += buf_str

            if is_last:
                current_entity.properties[prop_key] = current_property.substr(1, len(current_property) - 2)
                set_scope(FuncGodotMapParser.ParseScope.ENTITY)

I don't see an easy way to support this without breaking the rest of the parser. Also, considering that it's not recommended to have spaces in property names and to use either CamelCase or snake_case I'm admittedly not going to bother touching this myself.

I'll leave the issue open for now in case someone else wants to try to tackle this, but I'd honestly just recommend not actively avoiding good coding standards.

azur-wolve commented 4 weeks ago

not using spaced names is the correct

the concern is that if you end up releasing a game with support for user generated maps, then there's room for mappers to have issues with that (basically hidden non-obvious bugs)

so when a newcomer makes a map and tries to "compile" it, wont know easily why certain property is missing

RhapsodyInGeek commented 3 weeks ago

The FuncGodot Team decided pretty early on that we aren't going to worry about catering the plugin to newcomers. The most we'll likely do is just document that property names must not include spaces; any devs who try to allow user generated maps should also inform their users of that caveat. To me personally, it's like when users neglect to give their entities classnames, or when they forget to surround meta property strings with quotes: we don't have the manpower to worry about every single misuse of the plugin by inexperienced devs who don't understand proper coding fundamentals or who refuse to read the documentation.

All that said, if one of our community members wants to come up with something that will allow property keys with spaces to be parsed, I'm more than willing to review and merge it if it works.