import process bugs out if an entity script isn't a tool script

[Skaruts]

As I was writing this issue I noticed my code is slightly outdated, the latest code in question has changed and it seems to partially prevent this issue.

https://github.com/jitspoe/godot_bsp_importer/blob/512e256ec6c872f3a08d02e2ec41698b602bad2c/addons/bsp_importer/bsp_reader.gd#L758-L759

That second check actually prevents the importer from bugging out. But it still doesn't give you any feedback when you forget to make the script a tool script. I suppose that when you include that method, that means you want the script to be a tool script.

Maybe something like this could exist there?

if not scene.get_script().is_tool():
    printerr(scene.name + " has 'set_import_value' method, but script is not a tool script")
    # maybe no point continuing, since the method exists but itn't working?
    break 

printerr prints out a message to the output, but in red. I don't know if there's any better way to deal with it.

[jitspoe]

Thanks for pointing this out. I don't suppose there's any way to check that a function has a return type, is there? Would be nice to guarantee the function returns a bool, but this error at least provides some help!

Fixed in https://github.com/jitspoe/godot_bsp_importer/commit/f0681e5cd4182763142cd4d51f1480dd492373f8

[Skaruts]

Not that I know of. But I suppose you could explicitly check if it's not null and true. It won't be as elegant, but can do the job.

Tbh, I'm not too fond of the idea of being able to set actual node properties directly from spawnargs. I suppose it can make it easier to set up entities, but personally I prefer defining and controlling which "spawnargs" get used and everything else should get ignored. My end goal is to allow players to make their own maps, and I'm afraid that might be too much power on their hands. It might also require map makers to fix their maps, if they're using Godot properties that got renamed, changed, deprecated, etc.

At least that's the way I'm seeing it.

[jitspoe]

In that case, you could just have set_import_value() return true by default and nothing will be set that you don't explicitly check for. Note than only the @export vars will be set, so if you don't want people messing with internal variables, that shouldn't be an issue.

[Skaruts]

Note than only the @export vars will be set

True! I was just noticing that as I was trying to add in my first trigger and the settings weren't sticking until I exported the vars.

I guess my concerns are moot, and all is fine. :)