drwhut / tabletop-club

An open-source platform for playing tabletop games in a physics-based 3D environment for Windows, macOS, and Linux! Made with the Godot Engine.
https://tabletopclub.net
MIT License
1.27k stars 55 forks source link

Refactored AssetDB config file search to allow more wildcards #148

Closed elmodor closed 1 year ago

elmodor commented 1 year ago

Fixes/Solves The AssetDB config search was done via an recursive call. This is usually slow.

I tried to refactor the _get_file_config_value function. I tried two methods:

Both methods would allow different wildcards for the config file:

I compared the loading times:

Assets used:

While I would like a regex approach I don't think it is worth sacrificing loading times. Especially with loarget packs and more assets, it will take even longer.

Using the find method we are: faster, don't have a recursive call, and would be able to use wildcards at the beginning:

The method I used is:

What are your thoughts about this?

Would solve #146

drwhut commented 1 year ago

This is a great optimisation, thank you! I agree the find method seems to be best here, but I think I've found a couple of holes:

elmodor commented 1 year ago

I'm not sure if the in operator is enough here: e.g. if a file is called Item.test.png, and the config file has a section titled [*.test], then this would pick up that file, but should it? Should we only match the beginning and the end of the string?

So basically we would check:

var search_term = section.replace("*","")
if search.begins_with(search_term) or search.ends_with(search_term):

which is around 10ms slower than before. I think if there are ambiguous wildcards there's no way to know which one is the right one. What should [*.test] match to? It makes no sense without a file ending. It wouldn't match anything if it doesn't match Item.test.png. So not sure what the right move is here. Either one works and is faster than before.

With setting the config value at the end, would it not be better to go in reverse order, and break out the loop once a value is found (i.e. not null)?

I was thinking about it but decided to go for the easier and route. I tried it and I didn't noticed any speedup. Normally we only have like 1-2 found values anyway. To check if I need to break the for loop I need to do:

if value != default

Which leads to the issue that default and value are of different types (int, object, string) which threw an error. If we do some casting or more logic there I don't think we will gain any speed up at all.

drwhut commented 1 year ago

So not sure what the right move is here. Either one works and is faster than before.

Personally, when I see [*.test], that means to me any file ending with .test, rather than a file that has .test in the middle, which for me would make more sense to be caught by [*.test*], which... wait.

Could we check if it matches the beginning and/or the end depending on where the wildcards are? (in the example I gave, [*.test] would turn into .test, but only check the end of the string) But then do we want to include a sanity check to see if there is a wildcard in the middle?

Also, IIRC ConfigFile.get_value returns null if a section does not contain the given key, so I think you may need to check for this in the last for loop anyways.

EDIT: There's a method in ConfigFile for checking if a section has a given key!

elmodor commented 1 year ago

No the issue was that sometimes default value is a reference:

            var value = _get_file_config_value(config, from.get_file(), "value", Reference.new())
            var suit  = _get_file_config_value(config, from.get_file(), "suit", Reference.new())

this can not be compared to with an int (which is the correct return value).

Anyhow that's not needed anymore thanks to the section check.

So I updated both versions (is pushed). Regex has a global regex cache to avoid compiling the same regex all over again. This is quite the speedup. Regex is now ~ 330ms The other find (with checking whether the wildcard is at the beginning or end) is ~ 280ms

The regex might be a valid option after all? It would catch all possible wildcards.

EDIT: However, regex is also more dangerous. What if the filename is something [card?($%.png]. Works to load, kills the regex probably. And adding more security checks makes it just slower. So, maybe not

drwhut commented 1 year ago

Yeah, I think with regex there's going to be so many edge cases we need to worry about that I don't think it's worth implementing (plus, I think having the wildcards anywhere might be a bit overkill, the wildcards at the beginning and end work fine for 99% of use cases).

With the find method, we just need to test wildcards at the beginning, at the end, and at both ends.

elmodor commented 1 year ago

Cleaned up, removed regex and added documentation.