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

Make shared compstates copy cloneable types #5459

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

Conversation

metalgearsloth
Copy link
Contributor

Sometimes you can get trolled by stuff like autonetworked dictionaries not being copied on the client so you get mispredicts.

Downside is this allocates on server so not sure if we can just compile this out on server or what.

Sometimes you can get trolled by stuff like autonetworked dictionaries not being copied on the client so you get mispredicts.

Downside is this allocates on server so not sure if we can just compile this out on server or what.
Comment on lines 253 to 260
getStateInit.Append($@"
{name} = new(component.{name}),");
}
else
{
getStateInit.Append($@"
{name} = component.{name} == null ? null : new(component.{name}),");
}
Copy link
Member

@ElectroJr ElectroJr Sep 23, 2024

Choose a reason for hiding this comment

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

Only the client should have to clone for get-state (and it would only ever have to do so once per comp).
So this should somehow check is-server/is-client to avoid allocs.

And for fixture sys, its easy to manually add the checks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't debug sourcegen code coz my IDE blows up but not sure if I can just get the assembly or what, I'll see if I can figure something out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay I ended up using FULL_RELEASE but somehow it blows up tests. I think on an actual server it probably doesn't matter for getstate? Alternatively I use an msbuild property like pjb said.

Copy link
Member

@PJB3005 PJB3005 left a comment

Choose a reason for hiding this comment

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

I don't think FULL_RELEASE gets communicated properly to source gens? I'd rather have this relying on a compile property dynamically isnide the source gen.

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