refactor(pointsmaterial) : shared potree material among point tiles (no cloning)

[mbredif]

Description

Potree parsers used a hardcoded itowns.PointsMaterial material
This PR tackles some part of #695 to decouple the parsing and the styling of potree pointclouds.

Change

• Potree parsers now output a THREE.BufferGeometry, rather than a THREE.Points.
• The potree PointCloudProvider (to be renamed more explicitly as well as the pointcloud examples by the way :) ) is now responsible for creating the THREE.Points and adding the layer material (which defaults to itowns.PointsMaterial.
• customBinFormat is now a local variable (private implementation detail)
• realPointCount is removed as it was not used apparently (if needed, it can be accessed using the size of the position attribute)

Screenshots

No visible change

Question

I am not sure I understand why I need to clone the material. Ideally, I would like to use the same material for all pointcloud tiles.
@peppsac : any clue ?

[peppsac]

You could use this PR to work towards one important aspect needed for #695: unifying parser output.

So instead of adding a new possibility for parser output (we already have meshes and Features), I'd prefer that you use the same output as the geojson parser - improved if needed (maybe using a Float32Array for vertices + a crs attributes instead of Coordinates?).

[mbredif]

Here, I was more targetting the "Move symbolizing code out of formatters" facet of #695.
I would rather have simpler single-purpose PRs : unifying the parser outputs will need some discussions and touch more files... (and I agree that the current Feature is not suitable for pointclouds as we definitely need it to host typedarrays to enable direct uploading to the gpu whenever possible)

Concerning my question on the need to clone the material, I figured out that the issue was triggered by the pointcloud process that was setting pts.material.visible instead of pts.visible (see fa5f613).

• What is the rationale to set the material visibility (that could be shared by different objects) rather than the object visibility ?
• Would not it be simpler and more efficient if all the objects created by a layer shared a unique material layer.material, as sketched in fa5f613?

It might be that both per-layer and per-tile materials make sense depending on the context and that we might end up supporting both (eg by accepting layer.material as a material instance to be shared or as a material creator function called for each tile)

[peppsac]

If you want to use your own material, this PR is not needed, you can already do it:

layer.onPointsCreated = (layer, pts) => {
   pts.material = mymaterial;
};

That's why I'm suggesting to do something more useful for the long term. I don't request that you modify all parsers; you can simply improve the ones you're interested in (PotreeBinParser and PotreeCinParser).

I agree that the current Feature is not suitable for pointclouds as we definitely need it to host typedarrays to enable direct uploading to the gpu whenever possible

Great, then open another PR to fix this as well :)
Despite the smiley I'm serious: this would be useful for your issue and other parts of itowns.

[mbredif]

If you want to use your own material, this PR is not needed, you can already do it:

unfortunately, it is. Your snippet is likely not working, due to the current handling of the visibility in the pointcloud process (see fa5f613)...

Great, then open another PR to fix this as well :)

I can open an issue to start the discussion. Whenever someone has time to work on that, he will then have some material to base its work on...

[peppsac]

Your snippet is likely not working, due to the current handling of the visibility in the pointcloud process (see fa5f613)...

Can you elaborate (I don't understand the problem)?
The two visible come from three.js:

• mesh.visible means the mesh is visible (as in not culled)
• material.visible means the object will be displayed
  As far as I can tell, the PointCloudProcessing.js respects this convention.

[mbredif]

If you use a single material shared by all objects and the process modifies material.visible, then either all or no objects of the layer will be displayed (depending on the status of the last updated object)

mesh.visible means the mesh is visible (as in not culled)

indeed, that is lod-culling !

[mbredif]

#761 shows the shared material bug in action

[peppsac]

IMHO you need to clone the material. That's the way three.js works: if you use point.visible = false it'll also hide all its children.

So, with this error fixed in my snippet, you can do:

layer.onPointsCreated = (layer, pts) => {
   pts.material = mymaterial.clone();
};

[mbredif]

IMHO you need to clone the material

what do you mean by "need" ? this PR achieves its goal of a shared material decoupled from the parsers without cloning the material...

That's the way three.js works: if you use point.visible = false it'll also hide all its children.

...which does not matter here as all nodes are added to a flat group (no hierarchy), as far as I can tell. :)

[peppsac]

what do you mean by "need"

You need because of the meaning of the 2 visible property in three.js.

...which does not matter here as all nodes are added to a flat group (no hierarchy), as far as I can tell. :)

Yes, but you're only looking at itowns code. You don't know how it'll be used and you can't assume that users of itowns will never add children to points objects.

[mbredif]

I do not know what they are doing, but if user add children to points objects, which is not a documented behavior (and might interfere with pointcloudprocess logic), they might be happy to see that these objects get invisible whenever the points get invisible. That's actually a very good idea to simplify the box helper code, isn't it ? :)

If we cannot agree on the second commit for now, what do you think of the first one ?

[peppsac]

which is not a documented behavior (and might interfere with pointcloudprocess logic)

This is fully documented: it's the core behavior of three.js.

they might be happy to see that these objects get invisible whenever the points get invisible.

Sorry but this is wrong as well. We mark invisible points as invisible (here). But we also hide visible points when they are not needed.
This is not the same thing, and I don't think it's reasonnable for iTowns to mix the 2 concepts.

So not cloning the material would be handy for a few things but I don't think it's possible.

[peppsac]

If we cannot agree on the second commit for now, what do you think of the first one ?

I think I already answered to the first one here: #760 (comment)

[mbredif]

I understand that you prefer to close this PR unmerged.

• You recommend using the onPointsCreated hook rather than introducing a material creation parameter ?
• You do not want to decouple styling from parsing by leaving the dependency on PointsMaterial and the rendering logic in the potree parsers.

[mbredif]

The first part of this PR is ready to review : #764 (with 1 material per object).
This PR is now rebased on it (6084200) with a single material per layer (no cloning).

[peppsac]

Could you rebase this PR please?

[mbredif]

done! I added a second commit to fix some issues in the debug helper. I am not satisfied on the call to updatestats in the helper as this function is defined in the example. do you have a better proposition for fixing the display of the stats ?

[peppsac]

done!

Thanks.

I'm still not convinced that this is good idea because that goes against the logic exposed by threejs, so IMHO this shouldn't be merged.

[mbredif]

Not sure about what the 'logic of threejs' is, about sharing materials across objects. Do you have a pointer ?

Both approaches (master and this PR) work and offer different tradeoffs, depending if you want more or less to hide to the user that the layer is managing internally a collection of objects by exposing a single layer.material, and if you are more or less concerned by syncing issues for material properties :

• I think the current implementation works on stock threejs materials but the user have extra work to keep cloned materials (cf updateUniforms(), but that may not be sufficient if more than the uniforms need to be synced (defines, custom properties...), whereas this PR does not clone any material so no sync is required.
• for use cases that need to modify the material on a per-object basis you are likely using onPointsCreated so you can clone the material there and setup your custom update logic

let us keep this around for a while, until we settle on a common approach for all itowns materials :)

[peppsac]

Not sure about what the 'logic of threejs' is, about sharing materials across objects. Do you have a pointer ?

See #760 (comment), and #760 (comment) and #760 (comment)

In threejs, a material has per object properties (at the very least the visible property) so you can't use one instance for many objects.

Feel free to prove me wrong, but I don't want to merge something that breaks threejs semantics (again, especially for the .visible property).

[mbredif]

I meant pointers to comments not written by you ;). For reference, there are at least 2 visible properties : This PR shows that using the visible property from the object works as well. Let's not argue too much on for now and wait for use cases that would help us decide to merge or close this PR.

I found the pointcloud debug helper a little buggy (eg: sync issues for the stats and the visibility of boxes) and lacked the option to show only the sticky node. Would you be interested in writing a PR for the second commit only ?

[autra]

I found the pointcloud debug helper a little buggy (eg: sync issues for the stats and the visibility of boxes) and lacked the option to show only the sticky node.

untick "display parent" and "display children". It'll display only this node.

I think the current implementation works on stock threejs materials but the user have extra work to keep cloned materials (cf updateUniforms())

But that's a flaw to the particular solution chosen in #764 . In short, we (I included) have complicated our life on current master (properties are in layer.foo, then layer.material.foo, then layer.material.uniforms.foo, leading indeed to painful and difficult synchronization process). This can be a lot simpler, without requiring to keep the same material.

[mbredif]

layer.foo

I am for removing this one, and keep all stylization to layer.material, which could be renamed to layer.style to give it a more user-friendly name. Even layer.opacity could go there. (but that is already the discussion on the material/style normalization)

layer.material.foo

This one sounds good and should be kept. The good thing is that it does not impose anything on the material (it is free to use or not this new property).

then layer.material.uniforms.foo

The issue here is that materials may not have a uniforms property. Updating them should be a internal concern of the material

You are missing the syncing of the object.material(.uniform).foo of all tiles created by the layer if they are cloned (this PR is all about not having to care about this one !)

painful and difficult synchronization process

I agree, but it is only the downside of garanteeing the sync of the layer options with the object materials (I consider the previous behavior of only copying layer properties at tile creation time a bug). This PR is an alternative approach with the same sync garantee, there are surely other way to achieve this goal.

[gchoqueux]

this PR is rebased in #1589