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

Modify CesiumSubScene to use parent CesiumGeoreference's coordinates when added in-editor. #391

Merged
merged 5 commits into from
Jan 30, 2024

Conversation

azrogers
Copy link
Contributor

@azrogers azrogers commented Jan 18, 2024

Fixes #343.

When CesiumSubScene::OnEnable is called, it sets the coordinates of the parent CesiumGeoreference to the coordinates it has set. This means when adding a new CesiumSubScene component as a child of an existing CesiumGeoreference, it would reset the coordinates of the parent to the default coordinates. This change uses the Reset event to restore the georeference's original coordinates without changing the behavior outside of the editor.

@j9liu j9liu self-requested a review January 29, 2024 21:37
Copy link
Contributor

@j9liu j9liu left a comment

Choose a reason for hiding this comment

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

Thanks @azrogers ! I hadn't realized that OnEnable runs before Reset, so I can see how this became more complicated than anticipated.

Still I wonder if this can be simplified (ish)? Instead of storing the parent coordinates, is it possible to have a bool variable called isInitialized? (name can be improved if possible)

Then in OnEnable, you could have this code towards the end

if (Application.isPlaying)
  // In play mode, the subscene coordinates should be used as-is.
  this.isInitialized = true;

if (isInitialized) {
 this.UpdateOrigin()
 }

Then Reset could look something like this:

this.UpdateParentReference();
if(this._parentGeoreference != null) {
  // adopt parent georeference coordinates here
  isInitialized = true;
}

How does that sound? If it helps with clarity, we could potentially surround these segments with #if EDITOR statements.

I confirmed that this implementation works otherwise. I just think the internal state management is a little confusing.

Runtime/CesiumSubScene.cs Outdated Show resolved Hide resolved
@j9liu
Copy link
Contributor

j9liu commented Jan 30, 2024

Actually, the solution in my previous comment fails to account for existing sub-scenes when they're enabled in the editor. 🙃 And I don't know if the call is to make that isInitialized variable [Serialized]. So ignore what I wrote. I think the solution you found is fine; I'd like to add a bit of clarity to the comment but I'll put a suggestion tomorrow morning.

Just because it was driving me crazy, I looked up if there was an event for any time a component was added in the editor. I found that the ObjectFactory class exists and has a componentWasAdded event on it, but I have no idea when this event is called in relation to the component's event cycle. It's probably not worth exploring at this point.

Copy link
Contributor

@j9liu j9liu left a comment

Choose a reason for hiding this comment

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

@azrogers I left a few more comments, just for code clarity + organization.

Runtime/CesiumSubScene.cs Outdated Show resolved Hide resolved
Runtime/CesiumSubScene.cs Outdated Show resolved Hide resolved
Runtime/CesiumSubScene.cs Outdated Show resolved Hide resolved
Runtime/CesiumSubScene.cs Show resolved Hide resolved
@azrogers
Copy link
Contributor Author

@j9liu Updated review based on feedback - let me know if I missed anything!

@j9liu
Copy link
Contributor

j9liu commented Jan 30, 2024

Looks great, thanks @azrogers !

@j9liu j9liu merged commit db4df38 into main Jan 30, 2024
1 of 4 checks passed
@j9liu j9liu deleted the subscene-use-parent-coordinates branch January 30, 2024 21:48
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.

A new CesiumSubLevel should adopt the currently-active georeference origin
2 participants