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

refactor sq api init #461

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

refactor sq api init #461

wants to merge 4 commits into from

Conversation

uniboi
Copy link
Contributor

@uniboi uniboi commented Apr 28, 2023

Makes initializing the SquirrelManagers more readable by having definitions for CLIENT, UI & SERVER vm in one place.
Currently it looks like this

ON_DLL_LOAD_RELIESON("client.dll", ClientSquirrel, ConCommand, (CModule module))
{
	// ...

	g_pSquirrel<ScriptContext::CLIENT>->__sq_defconst = module.Offset(0x12120).As<sq_defconstType>();
	g_pSquirrel<ScriptContext::UI>->__sq_defconst = g_pSquirrel<ScriptContext::CLIENT>->__sq_defconst;

	// repeat for every function ...
}

ON_DLL_LOAD_RELIESON("server.dll", ClientSquirrel, ConCommand, (CModule module))
{
	g_pSquirrel<ScriptContext::SERVER>->__sq_defconst = module.Offset(0x12120).As<sq_defconstType>();

	// repeat for every function ...
}

The redundancy is annoying and makes it hard to see at a glance what the offsets for the functions are.

This PR implements this function that initializes the functions either on the CLIENT & UI or on the SERVER dynamically:

// Initialize a function on the CLIENT and UI or SERVER, depending on context
template <ScriptContext context, typename T>
void SetSharedMember(SquirrelManager<context>* manager, T* member, const CModule module, const int clOffset, const int svOffset)
{
	switch (context)
	{
	case ScriptContext::CLIENT:
	{
		*member = module.Offset(clOffset).As<T>(); // Set the member for the CLIENT vm
		*(T*)((char*)g_pSquirrel<ScriptContext::UI> + (int64_t)member - (int64_t)manager) = *member; // Set the member for the UI vm
		break;
	}
	case ScriptContext::SERVER:
		*member = module.Offset(svOffset).As<T>();
	}
}

Initializing a function for all vms now looks like this:

template <ScriptContext context> void InitSharedSquirrelFunctions(SquirrelManager<context>* manager, CModule module)
{
	SetSharedMember<context, sq_defconstType>(manager, &manager->__sq_defconst, module, 0x12120, 0x1F550);
}

The issue with this is that the memory layout of classes is ub afaik

Copy link
Member

@catornot catornot left a comment

Choose a reason for hiding this comment

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

works in testing, plugins still work too.
refactoring good :)

@RoyalBlue1 RoyalBlue1 self-requested a review April 29, 2023 10:59
@pg9182 pg9182 self-requested a review May 21, 2023 15:42
Copy link
Member

@pg9182 pg9182 left a comment

Choose a reason for hiding this comment

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

Doing this until I have time to look at it more closely.

  • Need to check each refactored line.
  • If the UB depends on the layout of TF stuff, it's fine, but if it's on our own classes, it's not worth it.

@GeckoEidechse
Copy link
Member

*bump*

@ASpoonPlaysGames ASpoonPlaysGames added merge conflicts Blocked by merge conflicts, waiting on the author to resolve almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge labels Sep 2, 2023
@GeckoEidechse
Copy link
Member

@uniboi just a heads-up that this PR has some merge conflicts atm

@GeckoEidechse
Copy link
Member

merge conflicts still

@GeckoEidechse
Copy link
Member

Converting to draft until merge conflicts are addressed. cc @uniboi

@GeckoEidechse GeckoEidechse marked this pull request as draft August 13, 2024 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge merge conflicts Blocked by merge conflicts, waiting on the author to resolve
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

6 participants