PolygonLayer- add elevationScale, fix updateTriggers.getElevation doesn't trigger updateGeometry #1046
Conversation
heshan0131
commented
Oct 17, 2017
heshan0131
commented
- bug fix: Recalculate geometry when updateTriggers.getElevation has changed
- add elevationScale
There was a problem hiding this comment.
There are 2-3 PRs here:
- A very straightforward addition of an
elevationScaleprop to the polygon layer.
2a) A tricky workaround for thegetElevationaccessor not being triggered correctly.
2b) A "hack" to the layer-browser to be able to trigger updateTriggers.
The first one I think could just be merged. The second one should ideally go through some discussion.
If these are being proposed for patch releases, having small PRs would also be a big benefit.
| // tessellator needs to be invoked | ||
| if (changeFlags.dataChanged || geometryConfigChanged) { | ||
| // check if updateTriggers.getElevation has been triggered | ||
| const getElevationTriggered = changeFlags.updateTriggersChanged && |
There was a problem hiding this comment.
While I think I understand why you needed to do this (you have an accessor that affects positions), you'll probably agree with me when I say that I don't like the amount of code needed to handle this case here.
We should take a second look at the PolygonTesselator it can handle this - to make sure we don't need it.
There was a problem hiding this comment.
Yes, this is what I had to do here to have polygon tessellation triggered by updateTriggers.getElevation. This bug affects both the Polygon layer and the GeojsonLayer. The frustration here is changeFlags.updateTriggersChanged only returns true or false, so there is no way to tell which accessor has changed. If there is way to tell whether updateTriggers.getElevation has changed in one line, it would be mostly ideal.
There was a problem hiding this comment.
I don't think this is the right approach.
With the current polygon layer setup, calculatePositions is already being called when getElevation update trigger changes, where we call this.state.polygonTesselator.positions(). What needs to happen is that we move the position calculation from the tesselator's constructor into this method.
There was a problem hiding this comment.
The tesselator is also a bit tricky as all other attributes depend on the positions. They need to be precalculated for the other attribute calculators to work. The model doesn't integrate smoothly with our normal attribute manager model.
I've made a couple of attempts to create a generic Tesselator/GeometryManager for layers with "custom geometries" to handle this (and a couple of other related issues) in a unified way but haven't had time to get it ready, unfortunately.
I guess we'll need to make a temporary fix here for now, proceed as makes sense. I'll approve the PR.
There was a problem hiding this comment.
We never use the positions array in other attribute calculation. Each attribute is calculated independently from the source data and accessors.
There was a problem hiding this comment.
To clarify - in a custom geometry the number of vertices per object is not constant. This is a calculation that depends on the "position" accessor (perhaps getPolygon or getPath) that affects all other attributes.
I tend to think of the number of verts that gets discovered during "position" calculation, an example is the PolygonLayer where we use something like the earcut library, although I think we are make some short cuts there.
In general one will need to either be able to calculate the number of verts in every attribute updater, or build an intermediary data structure to hold verts and indices and make it available to attribute updates. This is one of the problems I was tackling.
There was a problem hiding this comment.
The accessor that will change all attributes is getPolygon, whose update trigger we are not tracking, and that should be a bug.
The specific issue that this PR is trying to fix is regarding getElevation, which only affects positions, and should not need to reconstruct the tesselator.
There was a problem hiding this comment.
Of course. The main thing that concerns me is the amount of logic this is adding to the layers. I was trying to explain a solution I have been trying to build on the side. Have already approved this PR.
| lightSettings: LIGHT_SETTINGS, | ||
| elevationScale: 0.6 | ||
| }, | ||
| getUpdateTriggers: settings => ({ |
There was a problem hiding this comment.
Is this intended to create a manual test case for the getElevation updateTrigger?
Maybe we should add a separate button to trigger updateTriggers rather than hooking it to some arbitrary setting?
The prop handling in the layer browser is already very hacky. I would like us to at least think twice before adding more hacks.
There was a problem hiding this comment.
I agree this is hacky at a glance. At the moment, our layer browser doesn't provide any mechanism to allow tests on updateTriggers. It really increases the risks that updateTriggers are not handled correctly in layers. Though I agree with you this probably should not be included in this PR, will remove it and leave it for further discussion
heshan0131
changed the title
heshan0131
force-pushed
the
polygon-layer-elevation
branch
from
b7f557c
to
ad06d6d
Compare
…updateTriggers.getElevation has changed
heshan0131
force-pushed
the
polygon-layer-elevation
branch
from
ad06d6d
to
92dd041
Compare
…sn't trigger updateGeometry (#1046) * [PolygonLayer] add elevationScale * [GeoJsonLayer] add elevationScale * bug fix: [solidPolygonLayer] generate geometry when updateTriggers.getElevation has changed
…sn't trigger updateGeometry (#1046) * [PolygonLayer] add elevationScale * [GeoJsonLayer] add elevationScale * bug fix: [solidPolygonLayer] generate geometry when updateTriggers.getElevation has changed
