Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support multiple expressions in a 3D Tiles Style #5153

Closed
lilleyse opened this issue Mar 27, 2017 · 8 comments
Closed

Support multiple expressions in a 3D Tiles Style #5153

lilleyse opened this issue Mar 27, 2017 · 8 comments

Comments

@lilleyse
Copy link
Contributor

lilleyse commented Mar 27, 2017

@Dylan-Brown, @Jane-Of-Art for after #4759

Currently an expression may be computed by the style and referenced multiple times by the style to minimize computation and keep styles concise.

    "color" : {
        "expression" : "regExp('^1(\\d)').exec(${id})",
        "conditions" : [
            ["${expression} === '1'", "color('#FF0000')"],
            ["${expression} === '2'", "color('#00FF00')"],
            ["true", "color('#FFFFFF')"]
        ]
    }
}

It would be convenient to extend this to support multiple expressions, as outline here:
From CesiumGS/3d-tiles#2 (comment)

     "color" : {
        "expressions" : {
            "id" : "RegEx('^1(\\d)$').exec(${id})",
            "area" : "${length} * ${height}"
        },
        "conditions" : [
            ["(${ID} !== 1) && (${AREA} > 0)", "color('#FF0000')"]
        ]
    }

Most of the code changes will go in ConditionsExpression.js.

This will also include spec changes, so we'll need to be careful about certain details. For example, some expression names are reserved, like POSITION, POSITION_ABSOLUTE, NORMAL, and COLOR: https://github.com/AnalyticalGraphicsInc/3d-tiles/tree/master/Styling#point-cloud

Also should an expression called "id" be referenced by "${ID}" or "${id}" or "${_ID}" or "${_id}"? What happens when a feature has the same property name? This doesn't need to be decided right now, but should be considered.

@Jane-Of-Art
Copy link
Contributor

@lilleyse

"conditions" : {
            "(${ID} !== 1) && (${AREA} > 0)" : "color('#FF0000')"
        }

Should this be a string or a list of strings with keys?

@lilleyse
Copy link
Contributor Author

lilleyse commented Apr 3, 2017

Ah that comment is out of date now, it should be a list of string with keys. I'll edit the post.

@SunBlack
Copy link

Just to mention: "expression" is currently mentioned in spec, but not supported by PointClouds.

In general this is a really useful feature, but hard to implement, because you need to know result types of any operation to create new variables in glsl (there is no var like in JavaScript) - or you implement it just as string replacement, but then you have no performance benefit of this.

@lilleyse
Copy link
Contributor Author

lilleyse commented Apr 18, 2017

Just to mention: "expression" is currently mentioned in spec, but not supported by PointClouds.

It is supported, but only for conditions. Maybe that should change. Here's an example with the 3D Tiles Point Cloud Styling sandcastle:

addStyle('Secondary Color', {
    color : {
        expression : "vec4(${secondaryColor}, 1.0)",
        conditions : [
            ["${id} < 250", "${expression}"],
            ["${id} < 500", "${expression} * ${expression}"],
            ["${id} < 750", "${expression} / 5.0"],
            ["${id} < 1000", "rgb(0, 0, Number(${expression}.x < 0.5) * 255)"]
        ]
    }
});

Agreed that it could be hard to get the variable type, but I think it is possible with the existing code in getShaderExpression plus additional info about the types of the batch table properties.

Also good point about the performance implications. Right now the expression is handled by string replacement and it would be ideal for this to change.

@Dylan-Brown
Copy link
Contributor

Dylan-Brown commented May 19, 2017

An idea Jane gave for this:
Currently, the way the regular expression was found in her PR was to iterate over a list of known tokens - takes k*n time, for k known tokens and input of length n. As the number of possible tokens increases (ie. more than things like "temperature"), it would be more efficient to find all tokens and handling them that way as opposed to iteration over what is known in n time.

@lilleyse
Copy link
Contributor Author

Thanks @Jane-Of-Art and @Dylan-Brown for those notes, I'll keep that in mind for #5232

@lilleyse
Copy link
Contributor Author

lilleyse commented May 22, 2017

In order to make this easier to use for all styles, not just condition styles, the expressions probably belong outside of everything, like:

{
    expressions : {
        length : "length(${POSITION})",
        time : "${tiles3d_tileset_time} * 0.1"
    },
    color : {
        conditions : [
            ["${length} < 0.1", "${length} * color('red')"],
            ["${length} < 0.5", "vec4(vec3(${temperature} * mod(${time})), 1.0)"],
            ["${id} < 750", "vec4(${secondaryColor} / 5.0, 1.0)"],
            ["${id} < 1000", "rgb(0, 0, Number(${secondaryColor}.x < 0.5) * 255)"]

        ]
    },
    show : "${length} < 0.9"
}

I'll work on adding that to #5232

@lilleyse
Copy link
Contributor Author

lilleyse commented Jun 6, 2017

Fixed in #5232 but will rename expressions to defines.

@lilleyse lilleyse closed this as completed Jun 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants