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

WITCHER: Add CAuroraSettings data structure and initialization #527

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

rcxrdx
Copy link

@rcxrdx rcxrdx commented May 6, 2019

The CAuroraSettings data structure shared between C++/LUA is added from reversing the game in version 1.4.5.1304.
The fields have their original names (and thus original naming convention) to make the individual fields bindings easier.

@DrMcCoy
Copy link
Member

DrMcCoy commented May 8, 2019

Oh, neat, thanks! :)

(I'm a bit swamped with a move at the moment, so replies might be spotty. Sorry about that.)

However...I don't really want the original naming convention in xoreos' code. It clashes with our own naming conventions. Could you change that?

A few other things:

  • Could you meld the fixup in 2739aec into the commit that originally introduced that line? That way, our history stays clear and concise.
  • b3f459b and 7ed0934 also look like they should be one commit, not two.
  • Can you get rid of the merge commit and instead rebase your changing on top of the current master? If you don't know how to do that, no worries, I'll do that when I merge the PR once it's good to go.

@rcxrdx
Copy link
Author

rcxrdx commented May 9, 2019

Thanks for the comments, I have done those modifications.

@rcxrdx rcxrdx reopened this May 9, 2019
@@ -424,6 +424,667 @@ class LuaBindings {
static int luaBor(lua_State *state);
};

struct CAuroraSettingsData {
uint32_t _staticTimeFlow;
Copy link
Member

Choose a reason for hiding this comment

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

Can you use uint32 instead of uint32_t, to be consistent with the rest of the codebase?

(Yes, doing that was a mistake on my part, back when I started xoreos, but now we're kinda stuck with it.)

@@ -424,6 +424,667 @@ class LuaBindings {
static int luaBor(lua_State *state);
};

struct CAuroraSettingsData {
Copy link
Member

Choose a reason for hiding this comment

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

Just make it AuroraSettingsData, please.

uint32_t _speedTreeRenderBillboards;
uint32_t _dumpLoadingTextures;

CAuroraSettingsData();
Copy link
Member

Choose a reason for hiding this comment

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

This won't compile, since you add the constructor definiton in a later commit.

But also, can you do C++11-style inline initializations here instead? I.e.
float _attackLockDist { 6.0f };

@@ -142,11 +142,17 @@ void LuaBindings::registerBindings() {
LuaScriptMan.endRegister();
}

CAuroraSettingsData g_auroraSettings;
Copy link
Member

Choose a reason for hiding this comment

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

Does this have to be a global global? Can it be a static within LuaBindings::luaGetAuroraSettings() instead?

@DrMcCoy
Copy link
Member

DrMcCoy commented Aug 6, 2020

Hi @rcxrdx, are you still interested in working on this?

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.

None yet

2 participants