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

CuiHelper Json build optimisation. #488

Open
wants to merge 18 commits into
base: develop
Choose a base branch
from

Conversation

SkiTee3000
Copy link

Json build optimisation.
Added ability to send UI to the list of players.

According to my tests it reduces serialisation time up to 2-3 times.

Added ability to send UI to the list of players.
@Vlad-00003
Copy link
Contributor

Vlad-00003 commented Dec 19, 2023

Correct me if I'm wrong, but this way we loose
DefaultValueHandling = DefaultValueHandling.Ignore

In this case every field with default value would be send and it would totally mess up update = true, because every field that I didn't set manually would be updated to default values

@SkiTee3000
Copy link
Author

The default values will not be sent.
I have tested on all UI plugins I use, no problems found.

@Vlad-00003
Copy link
Contributor

Oh, my bad
I didn't notice that you're manually checking them in WriteJson

@MrBlue
Copy link
Member

MrBlue commented Jan 29, 2024

Hey, this looks interesting but I'm a bit concerned about some of the default value handling. Some such as the AnchorMin/Max you're not sending if the value is set to "0 0" "1 1", however setting those will change the behavior in-game for some reason. Can we test and make sure the variables we're skipping are actually properly being defaulted on the client? The client code doesn't set the default if the field isn't send in most if not all cases.

@SkiTee3000
Copy link
Author

Hey, this looks interesting but I'm a bit concerned about some of the default value handling. Some such as the AnchorMin/Max you're not sending if the value is set to "0 0" "1 1", however setting those will change the behavior in-game for some reason. Can we test and make sure the variables we're skipping are actually properly being defaulted on the client? The client code doesn't set the default if the field isn't send in most if not all cases.

I have removed this handling of default values, there may indeed be problems when updating the UI.

@SkiTee3000
Copy link
Author

@MrBlue, could you take a look?

@MrBlue
Copy link
Member

MrBlue commented Jul 1, 2024

Hey @SkiTee3000 would you mind updating this again?
Not gonna merge it this wipe but want to try test it before next wipe to make sure there's no issues or breaking changes when we merge it.

@SkiTee3000
Copy link
Author

Hey @SkiTee3000 would you mind updating this again? Not gonna merge it this wipe but want to try test it before next wipe to make sure there's no issues or breaking changes when we merge it.

updated

@DezLife
Copy link
Contributor

DezLife commented Jul 15, 2024

@MrBlue Check It pls

@DezLife
Copy link
Contributor

DezLife commented Jul 24, 2024

@SkiTee3000 Update this for new cooldown fixes

@SkiTee3000
Copy link
Author

@DezLife, updated.
@MrBlue, can you check?

@DezLife
Copy link
Contributor

DezLife commented Jul 24, 2024

@DezLife, updated. @MrBlue, can you check?

He will add this in an update in the test build. So that it can be tested and then added to the main build

@SkiTee3000
Copy link
Author

I just did a benchmark, compared to the current implementation in Oxide the json build is 3 times faster and there is 10% less memory allocation.
But I can't say that the results of my benchmark are super accurate, it was done on test data, not on any real ui.

@DezLife
Copy link
Contributor

DezLife commented Jul 24, 2024

I just did a benchmark, compared to the current implementation in Oxide the json build is 3 times faster and there is 10% less memory allocation. But I can't say that the results of my benchmark are super accurate, it was done on test data, not on any real ui.

This implementation will in any case speed up json serialization and reduce allocations

@DezLife
Copy link
Contributor

DezLife commented Jul 26, 2024

@SkiTee3000
Add also the ability to DestroyUI for the list of players

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants