Rework style #2006

ftoromanoff · 2023-02-14T16:03:04Z

Description

Change how itowns.Style() is used in relation with itowns.Feature.

remove propertie 'style' at some level to keep to the miminum:
- 2 were kept: layer and feature
- 2 were removed: collection and geometry.properties
make prioritarization of style work : layer (user) > feature (data) and this for all sub properties of style.
- currently, the layer style is not always applied over the data style. (see example 1)
- when one properties of point, stroke or fill is given, it all needed to be given or the default value will be used even if the child properties is existing.

Additional changes :

The instanciation of itowns.style is now only done on converter and not anymore on Source nor during the Feature generation:
For that we currently create an object style, to store all properties used later on for the instanciation of itowns.Style().
A slight change in base_altitude has been made, to be able to use a function for feature.style.
All references to parent Style had been removed.
Merge Style.symbolStylefromContext() with Style.drawingStylefromContext()

Examples:

source_file_kml_raster -> adding text.color: 'red'

in master branch:

with that PR:

examples/source_file_gpx_3d.html

src/Converter/Feature2Mesh.js

src/Converter/Feature2Texture.js

src/Core/Style.js

src/Layer/ColorLayer.js

src/Layer/LabelLayer.js

src/Layer/Layer.js

jailln · 2023-12-04T11:04:58Z

src/Core/Style.js

@@ -138,10 +138,10 @@ function defineStyleProperty(style, category, parameter, userValue, defaultValue
        {
            enumerable: true,
            get: () => {
-                if (property) { return property; }
-                if (userValue) { return readExpression(userValue, style.context); }
+                if (property !== undefined) { return property; }


you have to check for null too

done. Using value != undefined.

Seems to work for null and undefined but are you sure it won't be true in any other case? I haven't found many documentation about it and it is more generally done with property !== undefined && property !== null (even if it is more verbose)

I got the information on internet https://codedamn.com/news/javascript/check-if-undefined-null and checked it with https://developer.mozilla.org/fr/docs/Web/JavaScript/Reference/Operators/Inequality.

jailln · 2023-12-04T11:55:54Z

src/Core/Style.js

@@ -895,6 +895,13 @@ class Style {
                }
            }
        }
+        // VectorTileSet: by default minZoom = 0 and maxZoom = 24
+        // https://docs.mapbox.com/style-spec/reference/layers/#maxzoom and #minzoom
+        // Should be move to layer properties, when (if) VectorTile will be considered as multiLayer.


what do you mean by "multilayer" ?

A VectorTileSource send for each tile data on several 'layer' (building, road etc for instance), thus the use of the propertiy filter (that should be move to layer in my opinion). So one Layer with a VectorTileSource will have data on several vectorTile 'layer' that can have diferent 'layer' properties (style, zoom max and min etc). That why i fell like this kind of source is more a multiLayer that a real Layer.

Ok I understand what you mean, there is itowns layer concept and mapbox layer concept; can you precise your comment then or open an issue please? Otherwise I'm afraid that we won't understand this comment in a few month...

jailln · 2023-12-05T16:50:31Z

src/Core/Style.js

+ * and zoom of a Style instance.
+ * When the getter of a categorie.parameter is called, the purpose is to send, in that order, the value set by the user 'userValue'
+ * (if available), or the value read from the data 'dataValue' or the default value 'defaultValue' (if set). In case we
+ * need to overpass all we can set a value using the setter.


I propose to update the comment a bit:

Defines a new style property and its getter and setter for a given parameter in a given category (one of fill, stroke, point, text, icon or zoom). The getter is in charge of returning the right style value from the following ones if they are defined (in that specific order): * the value set by the user (`userValue`) * the value read from the data source (`dataValue`) * the default fallback value (`defaultValue`)

Thanks for that proposal that is indeed clearer that mine. I used it to update the documentation.

jailln · 2023-12-05T16:53:11Z

src/Main.js

@@ -39,7 +39,7 @@ export { CAMERA_TYPE } from 'Renderer/Camera';

 // Internal itowns format
 export { default as Feature, FeatureCollection, FeatureGeometry, FEATURE_TYPES } from 'Core/Feature';
-export { default as Style } from 'Core/Style';
+// export { default as Style } from 'Core/Style';


why do you need this change ? I think it is out of scope of this PR and we should uncomment it for now.

Indeed. It's not in the scope of the PR and was just a test to see if it can be removed safely in the futur (with the actual modification).

…at Feature level

…mContext(ctx) for hierarchization of style properties

…ntext()

ftoromanoff changed the title ~~Rework style~~ WIP Rework style Feb 14, 2023

ftoromanoff marked this pull request as draft February 14, 2023 16:03

ftoromanoff force-pushed the reworkStyle branch 12 times, most recently from aaa8c17 to c8d3ce6 Compare February 17, 2023 17:06

ftoromanoff force-pushed the reworkStyle branch 6 times, most recently from 4fb2b18 to 25f76f0 Compare March 15, 2023 14:37

ftoromanoff changed the title ~~WIP Rework style~~ Rework style Mar 15, 2023

ftoromanoff force-pushed the reworkStyle branch 8 times, most recently from 8ae0ae6 to 99a9d36 Compare March 27, 2023 13:36

ftoromanoff force-pushed the reworkStyle branch from 99a9d36 to c54fedd Compare April 19, 2023 12:18

ftoromanoff force-pushed the reworkStyle branch 6 times, most recently from 84f71b6 to fb2895d Compare November 21, 2023 14:04

ftoromanoff requested review from jailln and Desplandis November 24, 2023 10:48

jailln reviewed Nov 28, 2023

View reviewed changes

jailln reviewed Dec 4, 2023

View reviewed changes

ftoromanoff force-pushed the reworkStyle branch from c7d80eb to 65003a9 Compare December 5, 2023 15:59

jailln reviewed Dec 5, 2023

View reviewed changes

jailln approved these changes Dec 6, 2023

View reviewed changes

ftoromanoff force-pushed the reworkStyle branch 3 times, most recently from 251ef1f to b9287ba Compare December 7, 2023 14:39

ftoromanoff added 11 commits December 13, 2023 15:30

chore(Feature): rename base_altitudeDefault to camelCase

4c050c1

refactor(Style): change setFromGeojsonProperties() to static

a6c3503

refactor(Style): change Style.setFromVectorTileLayer to static

f1a9212

refactor(Style): supp collection.style and delete notion of style.parent

cc3c169

refactor(Feature2Mesh): add gestion feature with variable size

e007b2c

refactor(Feature): remove geometry.properties.style -> use style fct …

e2d46f0

…at Feature level

refactor(StyleContext): add setFeature to access feature.type

13511ee

refactor(style): change Style.drawingFromContext(ctx) to Style.getFro…

eb9cc78

…mContext(ctx) for hierarchization of style properties

refactor(Style): supp getTextFromProperties() ad it's done with getCo…

30904d8

…ntext()

refactor(Style): Style hierachisation in Layer.Style instanciation

9a65aef

refactor(FeatureToolTip): update with new gestion of Style

74f364e

ftoromanoff force-pushed the reworkStyle branch from b9287ba to 74f364e Compare December 13, 2023 14:46

ftoromanoff merged commit 356e695 into iTowns:master Dec 13, 2023
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework style #2006

Rework style #2006

ftoromanoff commented Feb 14, 2023 •

edited

Loading

jailln Dec 4, 2023

ftoromanoff Dec 5, 2023

jailln Dec 5, 2023

ftoromanoff Dec 6, 2023

jailln Dec 4, 2023

ftoromanoff Dec 5, 2023

jailln Dec 5, 2023

jailln Dec 5, 2023

ftoromanoff Dec 6, 2023 •

edited

Loading

jailln Dec 5, 2023

ftoromanoff Dec 6, 2023

Rework style #2006

Rework style #2006

Conversation

ftoromanoff commented Feb 14, 2023 • edited Loading

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ftoromanoff Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ftoromanoff commented Feb 14, 2023 •

edited

Loading

ftoromanoff Dec 6, 2023 •

edited

Loading