Skip to content

Add Config Node wrapper for easier config value access#168

Open
thelivingdiamond wants to merge 10 commits intomainfrom
ConfigNodeImprovements
Open

Add Config Node wrapper for easier config value access#168
thelivingdiamond wants to merge 10 commits intomainfrom
ConfigNodeImprovements

Conversation

@thelivingdiamond
Copy link
Owner

@thelivingdiamond thelivingdiamond commented Apr 26, 2025

I wanted to simplify the DX for accessing config node values after actually trying to use the old system in a mod.

I still am not sure if is this enough of an improvement to make breaking changes to the old config access system. It's basically just a structured wrapper around the pugi xml node. One more downside with the new implementation is it doesn't currently provide a way to notify the config manager that a config is dirty.

@thelivingdiamond thelivingdiamond requested a review from tmp64 April 26, 2025 19:01
@thelivingdiamond
Copy link
Owner Author

Did a refactor to see what it would look like if we completely removed the old config system and switched to the this one. would be mod SDK/API breaking.

Some unit tests would also be a good idea for this implementation just to make sure everything works

@tmp64
Copy link
Collaborator

tmp64 commented Apr 27, 2025

The new API is so much better. For future-proofing, this implementation can be split into two parts:

  • Actual implementation in Src/Common.Private/Chairloader, which will be shared between ChairManager, Chairloader and Preditor and will be used internally by them.
  • Mod SDK interfaces with version in Common/Chairloader (named like IConfigNode001)

Mod SDK interfaces will be wrappers around actual implementation. We can change the implementation in any way and not break mods. That will require returning something like std::unique_ptr<IConfigNode001> from config interfaces, which is ugly. But probably better than breaking mods

@thelivingdiamond
Copy link
Owner Author

Is the 001 part at the end of IConfigNode supposed to be versioning? i.e. if we change the implementation in the future we would add IConfigNode002?

@tmp64
Copy link
Collaborator

tmp64 commented Apr 27, 2025

Correct. Both 001 and 002 would need to be implemented to support old mods

@thelivingdiamond
Copy link
Owner Author

from what I can tell, switching to the interface breaks the cool [] operator and also means no more as() type functions bc virtual templated functions aren't a thing in c++. Am I missing something obvious to make that stuff work or is that just the tradeoff for oop here

@tmp64
Copy link
Collaborator

tmp64 commented Apr 27, 2025

Yeah, it'll get ugly

For operator[], you can make a custom ConfigPtr<T> that contains std::unique_ptr<T> and implements operator*, operator-> and operator[]

For as<T>(), you can add non-templated method that returns ValueType and implement templated functions in the interface

@thelivingdiamond
Copy link
Owner Author

I managed to get everything mostly as it was with some shenanigans. Need to run through and add documentation once we're generally happy with it

@thelivingdiamond
Copy link
Owner Author

Still not sure if we should use 64 bit types for numbers instead of int/uint/float

@thelivingdiamond thelivingdiamond requested a review from tmp64 May 17, 2025 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants