Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] cvar changes (DO NOT MERGE) #137

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

Rozelette
Copy link
Contributor

@Rozelette Rozelette commented Feb 28, 2023

This is a WIP example of reworking CVars to solve several issues with them. This is the LUS side of things, for the SoH PR, see HarbourMasters/Shipwright#2552.

The issues I am trying to fix:

  • CVar lookup is expensive since it involves a map lookup. It is one of the most expensive things that occurs in the game update loop (so outside of graphics/rendering)
  • CVar usage is error-prone. Since they are identified with strings, and defaulted if they don't exist (a common occurrence since they don't exist until they are changed), there is potential for misspellings to result in bugs that go undetected.

To remedy this, I have introduced a new interface for CVars. The core idea is to disassociate CVars and their storage of data. That is, there are CVars and store the value they name, and also ones that just associate that name to a value stored elsewhere. This enables CVars to just be used as a name for a regular C variable, allowing clients of CVars to safely use that data without any additional overhead, whether that overhead be in access time or bug potential.

This PR adds this new interface alongside the old so I could used and test it, so it is a bit awkward. The intent is to entirely replace the old one, so in judging it, I would ask you to consider only the new additions, and not the modifications to the existing interface. I will try to note points of interest.

Comment on lines +10 to +37
typedef struct Color_RGB8_t {
uint8_t r, g, b;

#ifdef __cplusplus
bool operator==(struct Color_RGB8_t const& other) const {
return (r == other.r) && (g == other.g) && (b == other.b);
}

bool operator!=(struct Color_RGB8_t const& other) const {
return !(*this == other);
}
#endif

} Color_RGB8;

typedef struct {
typedef struct Color_RGBA8_t {
uint8_t r, g, b, a;

#ifdef __cplusplus
bool operator==(Color_RGBA8_t const& other) const {
return (r == other.r) && (g == other.g) && (b == other.b) && (a == other.a);
}

bool operator!=(Color_RGBA8_t const& other) const {
return !(*this == other);
}
#endif

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes were to make these types comparable, for CVarDefaulted::ShouldSave()


namespace Ship {
typedef enum class ConsoleVariableType { Integer, Float, String, Color, Color24 } ConsoleVariableType;
typedef union CVarValue {

} CVarValue;

class CVarInterface {
public:
using VauleType = std::variant<int32_t, float, std::string, Color_RGBA8, Color_RGB8>;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In lieu of an enum to store the type, I opted for std::variant. This makes it more type safe and allows everything to be templatized.

};

template<class StorageType>
class CVarDefaulted : public CVarInterface {
Copy link
Contributor Author

@Rozelette Rozelette Feb 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This stores a CVar that has a default value. This is so we know to save this CVar only when it is not its default. This is to answer a question, "When asked to save, when should we save a CVar?" This is a question because these changes require a user to register all cvars in advance, so we don't necessarily want to save all of them if many of them are unchanged since they were registered.

This behavior was a struggle to me, since we are somewhat messy about things as it is. There are two possibilities:

  • Save when they are not the default
  • Save if they have ever been set (not just initialized), even if they are their default value.

I opted to do the first option since it allows use to change defaults if we ever decide a different default than what we had in a previous version is the best option for users. I am open to other opinions, however.

@@ -99,6 +99,8 @@ void Window::Initialize(const std::vector<std::string>& otrFiles, const std::uno
InitializeControlDeck();
InitializeCrashHandler();

CVarLoad();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was moved from the ImGui Init() because it made absolutely no sense there.

@@ -30,4 +30,5 @@ DEFINE_HOOK(GfxInit, void());
DEFINE_HOOK(ExitGame, void());
DEFINE_HOOK(LoadFile, void(uint32_t fileNum));
DEFINE_HOOK(DeleteFile, void(uint32_t fileNum));
DEFINE_HOOK(CVarInit, void());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The purpose of this hook is to allow clients to add their CVars after the CVar system has been set up, but before the config file has been loaded and the saved CVars are applied

@Kenix3 Kenix3 force-pushed the main branch 2 times, most recently from a3f18f7 to c946c2c Compare May 25, 2023 01:43
@Kenix3 Kenix3 marked this pull request as draft September 13, 2023 04:04
@Kenix3
Copy link
Owner

Kenix3 commented Dec 13, 2023

Any interest in seeing this PR to completion?

@Rozelette
Copy link
Contributor Author

Yes!

It has been a while since I've thought about this PR, and I have some new ideas for it.

I want to further move towards CVars just being a name to point to one of several kinds of data. So, CVars are just a fancy way to dereference a pointer. Now, what they point to can be one of several things.

The things they point to have two strata: if the data is static, and if the data is saved. Static meaning the data is known at compile time and is enumerated. And the data being saved is self-explanatory. This gives us some categories:

Settings - static and saved
Globals - static and not saved
Dynamic Settings - dynamic and saved
Console Variables - dynamic and not saved

Currently, all of our existing CVars are what I now call "Dynamic Settings." I want to move them towards "Settings," and then move some to "Globals" as we decide they don't have to be saved.

I will work towards a v2 of this rework soon.

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