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

Support no domain reload #417

Merged
merged 6 commits into from Feb 7, 2020
Merged

Support no domain reload #417

merged 6 commits into from Feb 7, 2020

Conversation

@huwb
Copy link
Collaborator

huwb commented Feb 6, 2020

Status

Ready to merge.

One of our users needs this with a priority if possible. @daleeidd if you're working today could you please give this a review? If not it's fine. Thanks!

What / why

The speedup entering/exiting playmode when domain reload is disabled in 2019.3 is incredible.

As noted in #373, crest did not support this option before now.

If the domain is not reloaded, any static variables will retain there value across the transition. There is a special callback that can be used to initialise these variables if needed. More info: https://docs.unity3d.com/2019.3/Documentation/Manual/DomainReloading.html

How

Some of our statics don't need to be reset across reloads. Some statics didnt need to be statics at all (for example, some shader param ids where private, and members of a class that were basically a singleton, so there was no point making them static - e.g. sp_LD_TexArray_Flow).

The remaining statics received (re)initialisation in the callback. The callback isn't usable pre-2019.3, so i've left initialisation both in line in the variable declaration, and in the callback function.

I could have ifdef'd out the entire callback function (InitStatics) each time, but TextureArrayHelpers actually needs to call the function as it has non-trivial initialisation, so i decided to go with this.

Also, TextureArrayHelpers used to depend on OceanRenderer from static functions which was a weird dependency, because it created a texture array with the current lod count slices, and the current lod resolution. I realised i could just create it as (4, 4, MAX_LOD_COUNT), where 4 is a 'small number' (2 may have worked as well, or maybe 1, but i seem to recall 4 being a min size for some gpu related things like compression, and thought it may as well make sense to use this). This saves memory, probably improves perf, and removes the dependency.

(A final note, it would be good agree a coding standard for statics (perhaps here: https://github.com/crest-ocean/crest/blob/master/CONTRIBUTING.md ). I think they were originally s_ but have evolved a bit organically and there is a range of things like no prefix, _, s_, sp_, and krnl_. I started setting them to s_ but stopped as i think it would be best to agree something we're all happy with, and it probably deserves a new PR).

Tests

I ran the example scenes in both 2018.4 and 2019.3 and all seems fine here. I ran a standalone build from 2019.3 as well and it seemed to work.

Issues

None that i'm aware of, besides the pitfall of double initialisation, which i'm not sure can be easily resolved in a way that is elegant and works on versions pre-2019.3 and after.

huwb added 2 commits Feb 5, 2020
Mix of solutions. Sometimes static wasnt required. Sometimes
the static variables do not change so, could be made readonly
and not touched. Sometimes static vars re-init'd.
@daleeidd

This comment has been minimized.

Copy link
Collaborator

daleeidd commented Feb 7, 2020

Looks ok to me. It tested fine too. Hopefully I didn't miss something.

Couldn't collections like Dictionary be kept as readonly and just use dictionary.Clear() in InitStatics?

I think it would have been nice to keep the Shader.PropertyToID static for everything for consistency and intent. But that is a non issue so feel free to ignore.

Also, you have to handle Static Event Handlers (at the end). It looks like there is only one being used.

As far as static prefixes go, I believe code intelligence makes it somewhat obsolete. No prefix will reduce friction for new contributors. Microsoft recommends against for public static fields

Copy link
Collaborator

moosichu left a comment

I've not had a chance to go through everything. My personal opinion is that we should keep our dependency on callbacks for resetting state low, and should prioritise keeping things readonly over keeping them static (especially in classes we instantiate once). However, for statics which were accessed across class boundaries, a dependency system might be needed for this which is beyond the scope of this.

@huwb

This comment has been minimized.

Copy link
Collaborator Author

huwb commented Feb 7, 2020

Thanks both!

I responded to comments directly, thanks for those

Couldn't collections like Dictionary be kept as readonly and just use dictionary.Clear() in InitStatics?

Yeah tom made he same comment, i changed it to do it that way

I think it would have been nice to keep the Shader.PropertyToID static for everything for consistency and intent

Yeah its tricky and kinda contradicts the next comment below.. so ive left it as is

My personal opinion is that we should keep our dependency on callbacks for resetting state low

Yeah that was my approach as well, hopefully i've found a good balance. if anything looks like it should not be static in addition to the ones ive changed let me know

@daleeidd good catch on the static events. looking at that one revealed something pretty funky in the code - currently every ocean chunk registers the begin camera rendering event, only to set the same static reference. i.e. _camera is set to the same value dozens of times. I've made a change which first deregisters before registering, which means only one chunk will eventually register an event (and i've added that code to deregister as instructed by the documentation). maybe there is a more sane way to do this, i need to think about it if there's a better way as this is a bit odd.

thanks for thoughts on coding standards too, we can discuss on discord if that makes sense

@huwb

This comment has been minimized.

Copy link
Collaborator Author

huwb commented Feb 7, 2020

Ok the latest should reflect all the feedback, does it look ok to you guys?

I retested on 2018.4 and 2019.3, all example scenes, with and without domain reload, and it seems to work well. And a standalone build too.

@huwb

This comment has been minimized.

Copy link
Collaborator Author

huwb commented Feb 7, 2020

Merging with Tom's blessing

@huwb huwb merged commit 7c9b9e7 into master Feb 7, 2020
@daleeidd

This comment has been minimized.

Copy link
Collaborator

daleeidd commented Feb 7, 2020

I just realised that the static event handler wasn't necessary this time round since OnEnable and OnDisable works like normal. Sorry about that. Unless you use ExecuteInEditMode which then "There is no extra OnDisable/OnEnable calls for ExecuteInEditMode scripts".

It is probably still not a bad idea to have it since it doesn't hurt. RunOnStart is being called after OnEnable so it is probably deregistering the event handler. I'll change it tomorrow if you haven't already.

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

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.