Fix(pointcloud): incorrect loading mechanism & add layer events#2673
Fix(pointcloud): incorrect loading mechanism & add layer events#2673Desplandis merged 2 commits intoiTowns:masterfrom
Conversation
| if (commonAncestor) { | ||
| return [commonAncestor]; | ||
| } | ||
|
|
There was a problem hiding this comment.
Are we sure we want to removed that part (as you said it's never called) but an other solution might have been to try to make this part used.
There was a problem hiding this comment.
I would prefer to remove unused (pre-mature) optimisation for now, until we manage to have a perfectly stable implementation of the point cloud update mechanism. Moreover, I do not use this code in the future priority-based mechanism.
There was a problem hiding this comment.
I see, but we might ask @jailln and @gchoqueux to agree ? Just to be sure ?
| this.group.add(elt.obj); | ||
| elt.obj.updateMatrixWorld(true); | ||
| context.view.notifyChange(this); | ||
| this.dispatchEvent({ type: 'load-model', scene: pts, tile: elt }); |
There was a problem hiding this comment.
Currently this event type is only listened in OGC3DTilesLayer.js and OGC3DTilesDebug.js.
And I don't see here any addEventListener for these... Is it really useful to add them then ?
There was a problem hiding this comment.
I use them for debugging purpose and I think this would be a nice addition to users who want to change the material (model-load) or just if we wish to implement some sort of benchmarking.
There was a problem hiding this comment.
agreed with @Desplandis , always useful for users to have access to such events
There was a problem hiding this comment.
Ok, I just currently don't see any use of them, we just need to be sure that they will be documented ;-)
There was a problem hiding this comment.
Yes they indeed need to be documented :)
Desplandis
force-pushed
the
fix/pointcloud-update
branch
from
455c46d to
e285750
Compare
The previous implementation added the loaded and visible object directly to the scene bypassing the notifyChange mechanism. The updates from a common node ancestor were never triggered (no call to notifyChange) and have also been removed. The old mechanism directly Added the loaded object as visible into the scene and was not trigering the notify change mechanism. The preUpdate code was also removed since was never used (no notify change trigger with a node).
Desplandis
force-pushed
the
fix/pointcloud-update
branch
from
36092d4 to
e25e7f9
Compare
4 participants
Description
This PR fixes the current loading mechanism of pointclouds. Those were added directly visible on the scene on load and bypassed the
notifyChangemechanism.This PR also adds layer events for model load, error and dispose for end-users and debugging purpose (the events types were based on those of
OGC3DTilesLayer).Motivation and Context
This could maybe help #2665 #2666 (@ketourneau if you can rebase on this and see if it fixes at least the points pop?)
Based on #2672