feat: SyncVar hooks can have 2, 1 or no params #4077

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

Jump to bottom

Draft

imerr wants to merge 1 commit into master

from feat/flexible_syncvar_hooks

Draft

feat: SyncVar hooks can have 2, 1 or no params #4077

imerr wants to merge 1 commit into master from feat/flexible_syncvar_hooks

+237 −14

Conversation

@imerr

Copy link

Contributor

@imerr imerr commented Jan 6, 2026

The weaver does this by generating a shim method with 2 params that calls the actual hook method

Not 100% sure if we actually want this for an extra 200 lines of weaver code

Tested with both mono&il2cpp builds (not tested virtual/static hook calls yet, will do if we plan on merging this)

@imerr


 feat: SyncVar hooks can have 2, 1 or no params

41231d7

@imerr imerr requested a review from MrGadget1024

January 6, 2026 11:25

MrGadget1024

MrGadget1024 requested changes

Jan 6, 2026

View reviewed changes

Copy link

Collaborator

@MrGadget1024 MrGadget1024 left a comment

Choose a reason for hiding this comment

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

Syntax changes

Assets/Mirror/Tests/Editor/SyncVars/SyncVarAttributeHookTest.cs

Comment on lines +26 to +29

void OnValueChanged(int oldValue)

{

HookCalled.Invoke(oldValue);

}

Copy link

Collaborator

@MrGadget1024 MrGadget1024 Jan 6, 2026

Choose a reason for hiding this comment

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

Suggested change

void OnValueChanged(int oldValue)

{

HookCalled.Invoke(oldValue);

}

void OnValueChanged(int oldValue) => HookCalled.Invoke(oldValue);

Assets/Mirror/Tests/Editor/SyncVars/SyncVarAttributeHookTest.cs

Comment on lines +39 to +42

void OnValueChanged()

{

HookCalled.Invoke();

}

Copy link

Collaborator

@MrGadget1024 MrGadget1024 Jan 6, 2026

Choose a reason for hiding this comment

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

Suggested change

void OnValueChanged()

{

HookCalled.Invoke();

}

void OnValueChanged() => HookCalled.Invoke();

Assets/Mirror/Tests/Editor/SyncVars/SyncVarAttributeHookTest.cs

public int value = 0;

public event Action<int> HookCalled;

Copy link

Collaborator

@MrGadget1024 MrGadget1024 Jan 6, 2026

Choose a reason for hiding this comment

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

Remove blank line

Suggested change

Assets/Mirror/Tests/Editor/SyncVars/SyncVarAttributeHookTest.cs

Comment on lines +53 to +56

void OnValueChanged(int oldValue)

{

HookCalled.Invoke(1);

}

Copy link

Collaborator

@MrGadget1024 MrGadget1024 Jan 6, 2026

Choose a reason for hiding this comment

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

Suggested change

void OnValueChanged(int oldValue)

{

HookCalled.Invoke(1);

}

void OnValueChanged(int oldValue) => HookCalled.Invoke(1);

Assets/Mirror/Tests/Editor/SyncVars/SyncVarAttributeHookTest.cs

Comment on lines +58 to +61

void OnValueChanged(int oldValue, int newValue)

{

HookCalled.Invoke(2);

}

Copy link

Collaborator

@MrGadget1024 MrGadget1024 Jan 6, 2026

Choose a reason for hiding this comment

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

Suggested change

void OnValueChanged(int oldValue, int newValue)

{

HookCalled.Invoke(2);

}

void OnValueChanged(int oldValue, int newValue) => HookCalled.Invoke(2);

...irror/Tests/Editor/Weaver/WeaverSyncVarAttributeHookTests_IsSuccess/FindsHookWithNoParams.cs

Comment on lines +11 to +14

void onChangeHealth()

{

}

Copy link

Collaborator

@MrGadget1024 MrGadget1024 Jan 6, 2026

Choose a reason for hiding this comment

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

Suggested change

void onChangeHealth()

{

}

void onChangeHealth() { }

...irror/Tests/Editor/Weaver/WeaverSyncVarAttributeHookTests_IsSuccess/FindsHookWithOneParam.cs

Comment on lines +11 to +14

void onChangeHealth(int oldValue)

{

}

Copy link

Collaborator

@MrGadget1024 MrGadget1024 Jan 6, 2026

Choose a reason for hiding this comment

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

Suggested change

void onChangeHealth(int oldValue)

{

}

void onChangeHealth(int oldValue) { }

...rror/Tests/Editor/Weaver/WeaverSyncVarAttributeHookTests_IsSuccess/FindsHookWithTwoParams.cs

Comment on lines +11 to +14

void onChangeHealth(int oldValue, int newValue)

{

}

Copy link

Collaborator

@MrGadget1024 MrGadget1024 Jan 6, 2026

Choose a reason for hiding this comment

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

Suggested change

void onChangeHealth(int oldValue, int newValue)

{

}

void onChangeHealth(int oldValue, int newValue) { }

.../Editor/Weaver/WeaverSyncVarAttributeHookTests~/ErrorWhenNoHookWithCorrectParametersFound.cs

Comment on lines +10 to 13

void onChangeHealth(int someOtherValue, int moreValue, bool anotherValue)

{

}

Copy link

Collaborator

@MrGadget1024 MrGadget1024 Jan 6, 2026

Choose a reason for hiding this comment

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

Suggested change

void onChangeHealth(int someOtherValue, int moreValue, bool anotherValue)

{

}

void onChangeHealth(int someOtherValue, int moreValue, bool anotherValue) { }

.../Editor/Weaver/WeaverSyncVarAttributeHookTests~/ErrorWhenNoHookWithCorrectParametersFound.cs

Comment on lines +15 to 18

void onChangeHealth(int someOtherValue, int moreValue, int anotherValue, int moreValues)

{

}

Copy link

Collaborator

@MrGadget1024 MrGadget1024 Jan 6, 2026

Choose a reason for hiding this comment

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

Suggested change

void onChangeHealth(int someOtherValue, int moreValue, int anotherValue, int moreValues)

{

}

void onChangeHealth(int someOtherValue, int moreValue, int anotherValue, int moreValues) { }

@MrGadget1024

Copy link

Collaborator

MrGadget1024 commented Jan 6, 2026

Not 100% sure if we actually want this for an extra 200 lines of weaver code

It's only ~100 new loc excluding tests (which are great), and if we can deprecate the 2-param signature that will eventually remove almost as much code. Most cases don't need oldValue and newValue is always available in the SyncVar field itself because we always set that before invoking the hook.

Ideally, we should warn for 2-param hooks to advise users to have one param or none, while still allowing two params to compile and work as they do today so there's a transition window for existing projects.

Labels

None yet

3 participants

@imerr @MrGadget1024

Uh oh!

feat: SyncVar hooks can have 2, 1 or no params #4077

Are you sure you want to change the base?

feat: SyncVar hooks can have 2, 1 or no params #4077

Conversation

@imerr imerr commented Jan 6, 2026

Uh oh!

@MrGadget1024 MrGadget1024 left a comment

Choose a reason for hiding this comment

Uh oh!

@MrGadget1024 MrGadget1024 Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

@MrGadget1024 MrGadget1024 Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

@MrGadget1024 MrGadget1024 Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

@MrGadget1024 MrGadget1024 Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

@MrGadget1024 MrGadget1024 Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

@MrGadget1024 MrGadget1024 Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

@MrGadget1024 MrGadget1024 Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

@MrGadget1024 MrGadget1024 Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

@MrGadget1024 MrGadget1024 Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

@MrGadget1024 MrGadget1024 Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

MrGadget1024 commented Jan 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants