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

Migration to 3D Tiles renderer followup #2415

Merged
merged 4 commits into from
Oct 11, 2024

Conversation

jailln
Copy link
Contributor

@jailln jailln commented Sep 27, 2024

Followup on 3D tiles renderer migration - see v1 of #2225:

  • Update 3dtiles_loader example:
    • don't reload the page each time we change the url of the tileset
    • Add a terrain (world)
    • Remove the other 3D Tiles examples
    • Add buttons to load pre-defined 3D tiles datasets
  • Add a deprecation warning in C3DTilesLayer constructor: will be removed in 3.0
  • Add more doc on OGC3DTilesLayer

Notes:
I started implementing functional tests on this branch but haven't fully succeeded yet and don't have much more time to put into it now. Basically I wanted to click on each button of the example, wait until the camera has moved and the tileset is loaded and then pick the scene to verify the 3D tiles layer is there. I tried several solutions but didn't manage to click on the buttons (or at least it didn't trigger the corresponding load methods) but directly calling the methods bound to onclick works ok. The main pain point is that waitUntilItownsIsIdle does not work in this case. I believe it's because it's based on the command-queue-empty which is triggered before the tileset has finished loading since tileset loading is now managed by 3DTilesRenderer. We need to find a way to know when tileset loading is over in 3DTilesRendererJS and we may also need to account for that before dispatching command-queue-empty. @Desplandis @AnthonyGlt any thoughts on this?

@AnthonyGlt
Copy link
Contributor

AnthonyGlt commented Sep 30, 2024

Thank you for this !

I think that we should wait for the zoom to end and then wait for the tiles to load.
For the zoom, there are events triggered from lookAtCoordinate which is called when we zoom, like animation-ended.

Concerning the loaded tiles, I didn't find any event worthwhile to use from the lib. Could be interesting to open an issue for this.
We could try to access to these values tiles.stats.downloading == 0, tiles.stats.parsing == 0, tiles.visibleTiles.size > 0, but it means checking them at each rendering loop :/

EDIT:
There is this event tiles-load-end that looks like the one we need

@Desplandis Desplandis self-requested a review October 7, 2024 07:20
@Desplandis Desplandis self-assigned this Oct 7, 2024
@jailln
Copy link
Contributor Author

jailln commented Oct 9, 2024

@AnthonyGlt Thanks, using both these events does the job :)

@Desplandis I added functional tests and also updated the object returned by the picking to conform better to what's returned for other layers (types will be more than welcome for such cases :) )

@jailln
Copy link
Contributor Author

jailln commented Oct 10, 2024

Sooo, the tests pass locally but not on the CI 😅 I tried a few things to fix them on the ci but didn't succeed. If you guys have any idea let me know, otherwise I'll just remove them.

@Desplandis
Copy link
Contributor

Maybe a different version of node on the CI vs yours locally?

We have an open PR (#2268) which should update the version of node on the CI (any news @mgermerie?)

@jailln
Copy link
Contributor Author

jailln commented Oct 10, 2024

Maybe a different version of node on the CI vs yours locally?

We have an open PR (#2268) which should update the version of node on the CI (any news @mgermerie?)

That was I though too but I already verified I used the right version. What's kind of weird is that one of the three tests is passing while all the test use pretty much the same logic.

@jailln
Copy link
Contributor Author

jailln commented Oct 11, 2024

Sooo, I removed the functional tests for now 😅 I kept them here if we want to finish the work one day

@jailln jailln merged commit 3d89169 into iTowns:master Oct 11, 2024
9 checks passed
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.

3 participants