iTowns / itowns

A Three.js-based framework written in Javascript/WebGL for visualizing 3D geospatial data
http://www.itowns-project.org
Other
1.09k stars 293 forks source link

Update outdated documentation #2191

Open AnthonyGlt opened 11 months ago

AnthonyGlt commented 11 months ago

Following the recent changes, could be a good idea to read the doc and update it when necessary. I stumbled across this: Screenshot from 2023-09-13 12-15-32

which should be edit, using this

foo.addEventListener(
                itowns.C3DTILES_LAYER_EVENTS.ON_TILE_CONTENT_LOADED,
                updatePointCloudSize,
            );

This is just an example of potential update. This issue can be used to list the edits that must be done.

valentinMachado commented 11 months ago

Yes this documentation is outdated, I introduced this change with this PR and was not aware of this documentation. It should be updated with something as you propose except updatePointCloudSize signature should change from

updatePointCloudSize(tileContent);

to

updatePointCloudSize({ tileContent });

I am wondering if the aim of this function is right though since there is an attribute material in C3DTilesLayer which is overriding points material see, meaning the user could write something like:

layer.material.size = 3;

to achieve the same thing. But this behavior of C3DTilesLayer is not documented..

WDYT ?

AnthonyGlt commented 11 months ago

Thanks for the feedback @valentinMachado ! If I'm not mistaking, layer.material is not defined and when the material is created here, it isn't assigned to layer.material. It means that we should add this layer.material = material but in ReferencingLayerProperties, we do the assignment the other way around : material.layer = layer, this could result in a cyclic reference (not sure if it's really an issue). Or, since we do not have access to the current material (from my understanding), we could directly give a new material:

 function updatePointCloudSize({tileContent}) {
                this.material = new itowns.PointsMaterial({
                    size:10,
                    mode: this.pntsMode,
                    shape: this.pntsShape,
                    classification: this.classification,
                    sizeMode: this.pntsSizeMode,
                    minAttenuatedSize: this.pntsMinAttenuatedSize,
                    maxAttenuatedSize: this.pntsMaxAttenuatedSize
                });
            }

Which goes against the purpose of the Style definition that we should use. This is a topic that should be more discussed, maybe in a related issue :thinking: Either way, your suggestion is relevant.