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

Use ulong for assetid instead of Guid #522

Closed
wants to merge 5 commits into from

Conversation

paulpach
Copy link
Contributor

@paulpach paulpach commented Mar 2, 2019

Assetid is used to identify which prefab to spawn with a spawn message.

I can't imagine a game having more than 100 or so spawn networked prefabs. So having 128 bits for this is overkill. Even 32 bits would be more than enough.

In addition, Guid has the extra annoyance that unity will not serialize it, so we end up with an awkward conversion from string.

This PR simplifies asset id by using only 64 bits and using ulong instead of Guid for the asset id.

@paulpach paulpach requested a review from miwarnec March 2, 2019 03:06
@atlv24
Copy link
Contributor

atlv24 commented Mar 2, 2019

Looks good! I like it. At some point we can use incremental IDs instead of hashes, and that can cut the size of this down to uint easily, maybe even ushort. But this already is a significant improvement!

@atlv24
Copy link
Contributor

atlv24 commented Mar 2, 2019

Actually, I think this breaks the weaver. I didnt test it but I think theres some code about Guids that has to be removed. (The references to ReadGuid etc)

@paulpach
Copy link
Contributor Author

paulpach commented Mar 2, 2019

Actually, I think this breaks the weaver. I didnt test it but I think theres some code about Guids that has to be removed. (The references to ReadGuid etc)

Good call. Fixed.

@atlv24
Copy link
Contributor

atlv24 commented Mar 2, 2019

It would be really nice to have a typedef to keep track of which ulongs are Guids. Maybe it'd be worth it to have using Guid = System.UInt64; at the top of the files that use Guids for now? Or would that just be unnecessary confusion?

@paulpach
Copy link
Contributor Author

paulpach commented Mar 2, 2019

using Guid = System.UInt64; would be extremely confusing.

@miwarnec
Copy link
Collaborator

miwarnec commented Mar 2, 2019

according to a microsoft developer, this doesn't sound like a good idea: https://blogs.msdn.microsoft.com/oldnewthing/20080627-00/?p=21823/

8 bytes don't seem to be worth the worrying that comes with it.

@paulpach
Copy link
Contributor Author

paulpach commented Mar 2, 2019

That was a great read! I was just assuming GUID were random, now I know better.

@paulpach paulpach closed this Mar 2, 2019
@paulpach paulpach deleted the assetid branch April 2, 2019 14:28
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

3 participants