refactor: move geometric layers off views #794
Conversation
Move all contents of `createXXXLayer` in XXXView to XXXLayer in Prefab/XXX/. This declutters the XXXView files. This change applies for Globe, Planar and Panorama. In the same time, GeometryLayer has been extract from Layer and given some new methods which were repeated through each View.
src/Core/Layer/GeometryLayer.js
Outdated
|
|
||
| this._attachedLayers = []; | ||
| this.type = 'geometry'; | ||
| this.protocol = 'tile'; |
There was a problem hiding this comment.
Is GeometryLayer restricted to be used with the TileProtocol?
There was a problem hiding this comment.
Actually, it shouldn't have a tile protocol in geometryLayer. you would need another tiledGeometryLayer or tiledMeshLayer inherited of geometryLayer.
Ideally it would be necessary to remove the layer protocol, and to replace it a source (where source is object which makes it possible to obtain the data)
src/Core/Layer/GeometryLayer.js
Outdated
| return this._attachedLayers.length < count; | ||
| }; | ||
|
|
||
| GeometryLayer.prototype.onTileCreated = function onTileCreated(layer, parent, node) { |
There was a problem hiding this comment.
This does not use this. which layer is it ? I suggest you either use this instead or make it a static function.
There was a problem hiding this comment.
Good catch, I also removed parent as it is not used.
src/Core/Layer/GeometryLayer.js
Outdated
| } | ||
| }; | ||
|
|
||
| GeometryLayer.prototype.preUpdate = function preUpdate(context, layer, changeSources) { |
There was a problem hiding this comment.
-
same thing here (no use of
this) -
it relies on
layer.level0Nodeswhich is only relevant for tiled GeometryLayer. Maybe there is room to create a TiledGeometryLayer subclass.
There was a problem hiding this comment.
- removed
- yeah @gchoqueux said that as well, what do you suggest ?
There was a problem hiding this comment.
yes, i agree
you would need a meshLayerProcessing class consisting of static functions that would then be attached to the layer.
actually, this preUpdate should be specific to the quadtree / tiledMesh and should be in tiledMeshLayerProcessing
|
So I pushed some changes, but I'm not satisfied with moving from |
src/Core/Layer/GeometryLayer.js
Outdated
|
|
||
| this._attachedLayers = []; | ||
| this.type = 'geometry'; | ||
| this.protocol = 'tile'; |
There was a problem hiding this comment.
Actually, it shouldn't have a tile protocol in geometryLayer. you would need another tiledGeometryLayer or tiledMeshLayer inherited of geometryLayer.
Ideally it would be necessary to remove the layer protocol, and to replace it a source (where source is object which makes it possible to obtain the data)
src/Core/Layer/GeometryLayer.js
Outdated
| import { EventDispatcher } from 'three'; | ||
| import Picking from '../Picking'; | ||
|
|
||
| function GeometryLayer(id, object3d) { |
There was a problem hiding this comment.
logically, it should be called meshLayer
There was a problem hiding this comment.
you could use
class?
Wouldn't that be losing coherence with the actual code or do you wish to push the use of class elsewhere ?
logically, it should be called meshLayer
But then the type would be mesh, and I find it too specific.
src/Core/Layer/GeometryLayer.js
Outdated
| this._attachedLayers = []; | ||
| this.type = 'geometry'; | ||
| this.protocol = 'tile'; | ||
| this.visible = true; |
There was a problem hiding this comment.
would visibility and opacity not be rendering/style options?
There was a problem hiding this comment.
Yes for opacity, no for visibility imho.
src/Core/Layer/GeometryLayer.js
Outdated
| return this._attachedLayers.length < count; | ||
| }; | ||
|
|
||
| GeometryLayer.prototype.onTileCreated = function onTileCreated(layer, parent, node) { |
src/Core/Layer/GeometryLayer.js
Outdated
| } | ||
| }; | ||
|
|
||
| GeometryLayer.prototype.preUpdate = function preUpdate(context, layer, changeSources) { |
There was a problem hiding this comment.
yes, i agree
you would need a meshLayerProcessing class consisting of static functions that would then be attached to the layer.
actually, this preUpdate should be specific to the quadtree / tiledMesh and should be in tiledMeshLayerProcessing
src/Core/Layer/GeometryLayer.js
Outdated
| } | ||
| }; | ||
|
|
||
| export default GeometryLayer; |
There was a problem hiding this comment.
finally it might look like this
class meshLayer {
source: {
// how get data
},
processing: {
// processing attached to layer
},
style: {
// how render this layer
},
}
|
@mbredif @gchoqueux I understand the concern on introducing a |
As GeometryLayer is currently used by 3dtiles and pointcloud, you can't ignore them. WDYT? |
|
Closing this PR, I will do another one soon from another branch (see this more complete commit zarov@3af73b0) |
Move all contents of
createXXXLayerin XXXView to XXXLayer in Prefab/XXX/. This declutters the XXXView files. This change applies for Globe, Planar and Panorama.In the same time, GeometryLayer has been extract from Layer and given some new methods which were repeated through each View.
The idea here is to simplifies some files, and later move the related processing of each layer (GlobeTileProcessing, etc) inside either the Layer itself, or in the same folder, to group each "type" of view and its workings (this, but also Controls too) in the same spot. Note that this is a proposition.