itowns icon indicating copy to clipboard operation
itowns copied to clipboard

Update outdated documentation

Open AnthonyGlt opened this issue 1 year ago • 2 comments

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.

AnthonyGlt avatar Sep 20 '23 08:09 AnthonyGlt

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 ?

valentinMachado avatar Oct 02 '23 14:10 valentinMachado

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.

AnthonyGlt avatar Oct 04 '23 07:10 AnthonyGlt