Conversation
Desplandis
force-pushed
the
cleanup/layer
branch
2 times, most recently
from
b56d9e0 to
2c2e6bf
Compare
Desplandis
force-pushed
the
cleanup/layer
branch
6 times, most recently
from
8f932c2 to
2292cc4
Compare
|
@AnthonyGlt @mgermerie @HoloTheDrunk If anyone want to review this PR. Especially if you depend on some properties here. |
|
@Desplandis Thank you for the PR. |
| nodeLevel: number, | ||
| currentLevel: number, | ||
| failureParams: { lowestLevelError: number }, | ||
| zoom?: { min: number; max: number }, |
There was a problem hiding this comment.
zoom is bothering me a bit because it's only used for _dichotomy() but I don't have an idea to make it better so OK 👍
| */ | ||
| this.isColorLayer = true; | ||
|
|
||
| this.style = style instanceof Style ? style : new Style(style); |
There was a problem hiding this comment.
Wouldn't it make more sense to move style into Layer, in order to centralize common properties (visibility, opacity/scale, etc.) that every layer can have?
There was a problem hiding this comment.
Since not all layers are styled (e.g. elevation layers, pointcloud layers), I decided to remove those properties from the base Layer class.
| async startup(/* context */) { | ||
| try { | ||
| await Promise.all(this._promises); | ||
| this._resolve(); | ||
| } catch (error) { | ||
| this._reject(error); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
| async startup(/* context */) { | |
| try { | |
| await Promise.all(this._promises); | |
| this._resolve(); | |
| } catch (error) { | |
| this._reject(error); | |
| } | |
| } | |
| async startup(/* context */) { | |
| await Promise.all(this._promises) | |
| .then(this._resolve) | |
| .catch (this._reject); | |
| } |
There was a problem hiding this comment.
Depends on whether we want to use the async paradigm or not, but I will follow your suggestion here.
Desplandis
force-pushed
the
cleanup/layer
branch
from
53201cf to
ea6ac46
Compare
3 participants
Description
This PR introduces several changes to the base Layer and layer-related classes. The main changes includes the migration of
LayerUpdateStateandLayerUpdateStrategyto typescript, the reorganization of Layer properties for better separation of concerns and the addition of a startup lifecycle method as a first step to cleanup the mess that isView#preprocessLayer. This will not break existing layers within iTowns but could break custom layers inheriting directly fromLayer.Motivation and Context
The motivation for the following changes is as follow:
Layer#startuplifecycle method for separation of concerns, allowing for more explicit initialization of layers. Follow-up: removeView#preprocessLayerand most of the complexity ofView#addLayer.LayerUpdateStateandLayerUpdateStrategyto typescript (I will personally rebase Further simplify tile subdivision mechanism #2525 on this PR since there is some function paramater changes tochooseNextLevelToFetch).Follow-up: migrate TileMesh-related modules to typescript (see Migrate TileMesh-related modules to typescript #2557) / migrate
LayerNodeProcessingImageryLayersto its own module as I have short-term plan to migrate all layers to typescript (with some type hacks) and have no plan to migrateImageryLayers(which given the code needs an entire refactoring...).Follow-up: migrate Layer modules to typescript
Layerand children.layerUpdateStrategyused only onRasterLayer-derived classesstyleused only on*3DTilesLayer,FeatureGeometryLayerandColorLayermergeFeatures/addLabelused only onFeatureGeometryLayerandColorLayerFollow-up: migrate Layer modules to typescript
Changes considered but not done on this PR:
View#preprocessLayerScreenshots (if appropriate)