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

Fix OceanDepthCache disabling in edit mode #897

Closed
wants to merge 1 commit into from

Conversation

daleeidd
Copy link
Collaborator

This PR replaces #629.

The OceanDepthCache disabling itself in edit mode is the primary issue this PR addresses.

While #629 is a complete and robust solution, it is quite heavy for the three components that will use it:

  • OceanPlanarReflections
  • OceanDepthCache
  • UnderwaterEnvironmentalLighting

Disabling the component only saves Update or PreRender from running and for these components which normally only have one it should be fine (OceanDepthCache disables after start anyway). Also, OceanDepthCache hard dependent on OceanRenderer as it falls back to the transform height.

Instead this makes it so dependent components execute after the OceanRenderer and leaves managing delayed initialisation to the user if they really need it. This is done with DefaultExecutionOrder attribute. It is the same thing as setting the execution order in the project settings, except they don't show up in the list. It can be overridden by the user if they add their own entry.

Let me know what you think.

Also use DefaultExecutionOrder
@daleeidd daleeidd added the Bug label Aug 29, 2021
@daleeidd daleeidd requested review from huwb and removed request for huwb August 29, 2021 06:38
@daleeidd daleeidd marked this pull request as draft August 29, 2021 07:53
@daleeidd
Copy link
Collaborator Author

This was half thought out so converted to draft. Numerous components use the pattern which disables itself in Start if no OceanRenderer is present. So maybe the other PR is also useful still. I'll strip this down to fixing the depth cache only.

@huwb
Copy link
Contributor

huwb commented Aug 29, 2021

Fwiw I approved 629, think I'd be in favour of merging that but you will know best.

@daleeidd
Copy link
Collaborator Author

Fwiw I approved 629, think I'd be in favour of merging that but you will know best.

Thanks. I won't merge it for this release as I would like to add some unit tests for that PR before merging.

@daleeidd daleeidd closed this Aug 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants