League-of-Foundry-Developers / torch

Simple torch module for Foundry VTT
10 stars 20 forks source link

[SWADE] Light radius settings do not work #72

Open ddbrown30 opened 10 months ago

ddbrown30 commented 10 months ago

The settings for bright and dim radius in the module settings do not do anything in SWADE. They are set to 20/40 but the actual values being used are 4/4. I'm assuming that's because the values in sources.json take precedent over the option.

Actually, digging a bit more into the code, I'm not sure when these values would ever be used in any system. Maybe the settings should just be removed?

Edit: Ah, I'm guessing it's for when someone is using an unsupported system. In that case, my recommendation would be to hide the options when using a supported system.

lupestro commented 8 months ago

That’s a good point. Thanks for that. It would fit in well with the next set of changes that are on my slate. Hopefully, at the end of those changes, users will feel it is simpler and easier to understand.

Before I start the changes, I need to add a safety net of tests so I can easily tell when I’ve broken something. This weekend, I’ve just enhanced the test infrastructure, so next I need to use it to beef up the tests. Once I’ve woven the safety net, I will start the changes.

lupestro commented 7 months ago

Doing some work in tests today, I can confirm that the primary case where the settings for "inventory item name to use", "bright light radius", and "dim light radius" apply are when there is no configured data for the system.

There appears to be a secondary case, though, although I discovered I have no tests for it. Essentially, the user config settings override the out-of-the-box settings, but not the user-supplied sources but only for a single source.

The latter case is confusing and I'd love to get rid of it, perhaps then doing as you say, only offering those three game settings if there is no source data for the system you're running. Would only conditionally offering those settings be more confusing than offering them even on systems for which they don't apply? I'm not sure.

I welcome opinions from the users.