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

Trigger a rendering when a data source becomes ready #11934

Merged
merged 6 commits into from
Apr 29, 2024

Conversation

javagl
Copy link
Contributor

@javagl javagl commented Apr 15, 2024

Description

This only triggers a requestRender() when a DataSource becomes 'ready' for the first time, in DataSourceDisplay#update.

This fixes #11928
(Which was a regression from #11855 )

Testing plan

Until now, I tested this with the Sandcastle from the original issue. The behavior now is that when the data source finishes loading (as indicated by the 'Loaded' console message), a requestRender is triggered and the geometry appears without moving the camera.

I also tested it with the "Case 1, Primitive" sandcastle from the PR #11855, just to check that this behaves as before.

For unit tests: I'll have to check how this can be tested sensibly. There probably already is a test along the lines of

createViewerWith( requestRenderMode: true );
addSomeDataSource(...)
scene.postRender.addEventListener(() => {
    shouldBeCalled();
}

But I first wanted to confirm that this is the right "layer" of addressing this issue. (It could probably be solved in a million different ways, and it's not unlikely that there is a "better" way to solve it...)

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have update the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

Copy link

Thank you for the pull request, @javagl!

✅ We can confirm we have a CLA on file for you.

@ggetz
Copy link
Contributor

ggetz commented Apr 22, 2024

Thanks @javagl! I do think this is an appropriate fix for the specific issue.

Could you please update CHANGES.md?

This may be a bit tricky to test the specific of this fix given that completed requests will also trigger renders. One option would be to start listening once the datasource request is complete.

@javagl
Copy link
Contributor Author

javagl commented Apr 22, 2024

I updated the CHANGES.md, and added a test, roughly following the pattern from similar specs. Should I try to cover some flow like

when (becomes ready during update) expect(renderRequested to be true);
when (remains ready during update) expect(renderRequested to be false);

?
(It looks like scene.renderForSpecs() does not reset the scene._renderRequested flag, so I might have to look closer into that...)

@ggetz
Copy link
Contributor

ggetz commented Apr 22, 2024

Thanks @javagl! Any additional coverage would be appreciated.

Your point about the _renderRequested flag not being reset indicates that this feature probably isn't a finely covered in the unit tests.

@javagl
Copy link
Contributor Author

javagl commented Apr 24, 2024

Your point about the _renderRequested flag not being reset indicates that this feature probably isn't a finely covered in the unit tests.

Well, upon further investigation, it was reset to false, but then immediately set to true again, via a ... not-so-obvious mechanism that I described in a short inline comment. (If this falls into the category of "Don't pollute test code with information that everybody knows", I can remove it...)

@javagl
Copy link
Contributor Author

javagl commented Apr 25, 2024

@ggetz I don't see a connection between the changes in this PR and the current build failures. But if there could be a connection, I can have another look...

@ggetz
Copy link
Contributor

ggetz commented Apr 25, 2024

@javagl I agree the CI failure is unrelated. I'll open a new issue to track.

@ggetz
Copy link
Contributor

ggetz commented Apr 29, 2024

Thanks @javagl! This is looking good to me.

@ggetz ggetz merged commit 9663d7e into main Apr 29, 2024
9 checks passed
@ggetz ggetz deleted the fix-polyline-not-rendered-in-request-render-mode branch April 29, 2024 14:26
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.

Polyline Entity is not rendered in request render mode
2 participants