dialogic-godot / dialogic

💬 Create Dialogs, Visual Novels, RPGs, and manage Characters with Godot to create your Game!
https://dialogic.pro
MIT License
4.05k stars 239 forks source link

Proposal: Move all ProjectSettings access to a new DialogicSettings.gd file #2471

Open salianifo opened 3 weeks ago

salianifo commented 3 weeks ago

Is your feature request related to a problem? Please describe. While working on various fixes/features for Dialogic, I've noticed that a lot of Dialogic settings (ones using ProjectSettings) are accessed in multiple places, leading to default values needing to be defined in multiple places, as well as the string-based keys being all over the place leading to potential typos or repeated const definitions in multiple places.

Describe the solution you'd like I propose we move all of these into a new DialogicSettings.gd file as static get/set functions so that default values and string-based keys are only defined in one place and accessed as DialogicSettings.get_translation_enabled() instead of ProjectSettings.get_setting('dialogic/translation/enabled', false). This will minimize the chances of typos, and remove the need to change values in multiple places if a default value for something needs to be changed. It also allows us to utilize autocomplete more effectively when working on the Dialogic code.

Describe alternatives you've considered N/A

Additional context N/A

Jowan-Spooner commented 2 weeks ago

This has been considered before. However it kinda defeats the purpose of the modules, because it means they are no longer self-contained. The best alternative I can think of is putting the settings on the subsystems they most closely belong to and giving the subsystem a class name. However this would result in a bunch more class names (and dialogic is already flooding projects with a bunch of custom classes. It's unfortunate that we cannot mark classes as not to be exosed to the add node dialog).

salianifo commented 2 weeks ago

Yeah that was my original thought as well. I had first investigated about having them on the subsystems, only to later realize they aren't tool scripts (alongside the Dialogic autoload also not being a tool script).

I just thought of this while typing this so it might not be fully fleshed out, what if we made the DialogicSettings.gd have a generated list of the subsystems like DialogicGameHandler.gd does (or a new DialogicModuleSettings derived class instead of DialogicSubsystem) that can be added to the indexer. This way each module can have it's own settings accessed as DialogicSettings.Audio.get_whatever() and they can still be unnamed classes because the type can be bound using preload like the subsystems are. This way each module can have a module_settings.gd or something, and it can be added to that modules index.gd file. This would keep them self contained, while still providing the other benefits and only adding 2 new global class names. This also means that settings that don't currently have a module could be added to core, but with a different group name, for example translation settings could be under DialogicSettings.Translations.get_whatever(). This would allow them to be moved to a different module in the future if needed, without having to change how they are accessed everywhere.

I also heavily lament that fact that Godot doesn't have any kind of namespaces or way to limit global class name pollution which would help alleviate these kinds of problems from the start but, that's unlikely to be implemented anytime soon. :sob:

Jowan-Spooner commented 2 weeks ago

Well traits have been discussed for ages now, but yeah, unlikely they will happen soon. I kinda like your proposal. It does feel a bit over-kill, as it's a lot of effort for something that is already kinda working and just not pretty, but I could see it certainly being useful. And I certainly can't think of a much better version rn.

salianifo commented 2 weeks ago

I can move forward with an implementation if you want, after I finish the new audio event.