Skip to content

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 imerr requested a review from MrGadget1024 January 6, 2026 11:25
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

Comment on lines +26 to +29
void OnValueChanged(int oldValue)
{
HookCalled.Invoke(oldValue);
}
Copy link
Collaborator

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);

Comment on lines +39 to +42
void OnValueChanged()
{
HookCalled.Invoke();
}
Copy link
Collaborator

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();

public int value = 0;

public event Action<int> HookCalled;

Copy link
Collaborator

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

Comment on lines +53 to +56
void OnValueChanged(int oldValue)
{
HookCalled.Invoke(1);
}
Copy link
Collaborator

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);

Comment on lines +58 to +61
void OnValueChanged(int oldValue, int newValue)
{
HookCalled.Invoke(2);
}
Copy link
Collaborator

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);

Comment on lines +11 to +14
void onChangeHealth()
{

}
Copy link
Collaborator

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() { }

Comment on lines +11 to +14
void onChangeHealth(int oldValue)
{

}
Copy link
Collaborator

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) { }

Comment on lines +11 to +14
void onChangeHealth(int oldValue, int newValue)
{

}
Copy link
Collaborator

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) { }

Comment on lines +10 to 13
void onChangeHealth(int someOtherValue, int moreValue, bool anotherValue)
{

}
Copy link
Collaborator

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) { }

Comment on lines +15 to 18
void onChangeHealth(int someOtherValue, int moreValue, int anotherValue, int moreValues)
{

}
Copy link
Collaborator

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

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.

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.

3 participants