-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
docs: Viewport elevation control #9313
base: master
Are you sure you want to change the base?
Changes from all commits
9f8e01f
e6b304d
e20e91c
33dde58
9d8647a
e306dfe
5c14463
a8117e5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -143,7 +143,7 @@ export default class GridLayer<DataT = any, ExtraPropsT extends {} = {}> extends | |
getValue: ({positions}: {positions: number[]}, index: number, opts: BinOptions) => { | ||
const viewport = this.state.aggregatorViewport; | ||
// project to common space | ||
const p = viewport.projectPosition(positions); | ||
const p = viewport.projectPosition([positions[0], positions[1], positions[2] || 0]); | ||
const {cellSizeCommon, cellOriginCommon} = opts; | ||
return [ | ||
Math.floor((p[0] - cellOriginCommon[0]) / cellSizeCommon[0]), | ||
|
@@ -241,7 +241,10 @@ export default class GridLayer<DataT = any, ExtraPropsT extends {} = {}> extends | |
let viewport = this.context.viewport; | ||
|
||
if (bounds && Number.isFinite(bounds[0][0])) { | ||
let centroid = [(bounds[0][0] + bounds[1][0]) / 2, (bounds[0][1] + bounds[1][1]) / 2]; | ||
let centroid: [number, number] = [ | ||
(bounds[0][0] + bounds[1][0]) / 2, | ||
(bounds[0][1] + bounds[1][1]) / 2 | ||
]; | ||
const {cellSize, gridOrigin} = this.props; | ||
const {unitsPerMeter} = viewport.getDistanceScales(centroid); | ||
cellSizeCommon[0] = unitsPerMeter[0] * cellSize; | ||
|
@@ -272,7 +275,7 @@ export default class GridLayer<DataT = any, ExtraPropsT extends {} = {}> extends | |
binIdRange = getBinIdRange({ | ||
dataBounds: bounds, | ||
getBinId: (p: number[]) => { | ||
const positionCommon = viewport.projectFlat(p); | ||
const positionCommon = viewport.projectFlat([p[0], p[1]]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment as above, better to do |
||
return [ | ||
Math.floor((positionCommon[0] - cellOriginCommon[0]) / cellSizeCommon[0]), | ||
Math.floor((positionCommon[1] - cellOriginCommon[1]) / cellSizeCommon[1]) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -219,7 +219,7 @@ export default abstract class Layer<PropsT extends {} = {}> extends Component< | |
// Public API for users | ||
|
||
/** Projects a point with current view state from the current layer's coordinate system to screen */ | ||
project(xyz: number[]): number[] { | ||
project(xyz: [number, number, number]): [number, number, number] { | ||
assert(this.internalState); | ||
const viewport = this.internalState.viewport || this.context.viewport; | ||
|
||
|
@@ -230,20 +230,20 @@ export default abstract class Layer<PropsT extends {} = {}> extends Component< | |
coordinateSystem: this.props.coordinateSystem | ||
}); | ||
const [x, y, z] = worldToPixels(worldPosition, viewport.pixelProjectionMatrix); | ||
return xyz.length === 2 ? [x, y] : [x, y, z]; | ||
return [x, y, z]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I consider this a breaking API change. Could Not saying yet that we can't make this API change, but generally we'll tradeoff type safety to preserve the API, and not the other way around. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even that will be breaking change as many people out there are likely passing a function project(xyz: [number, number]): [number, number];
function project(xyz: [number, number, number]): [number, number, number];
function project(xyz: Vector2Or3): Vector2Or3 { There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also we haven't got around to using them everywhere yet, but we have helper types like |
||
} | ||
|
||
/** Unprojects a screen pixel to the current view's default coordinate system | ||
Note: this does not reverse `project`. */ | ||
unproject(xy: number[]): number[] { | ||
unproject(xy: [number, number, number]): [number, number, number] { | ||
assert(this.internalState); | ||
const viewport = this.internalState.viewport || this.context.viewport; | ||
return viewport.unproject(xy); | ||
} | ||
|
||
/** Projects a point with current view state from the current layer's coordinate system to the world space */ | ||
projectPosition( | ||
xyz: number[], | ||
xyz: [number, number, number], | ||
params?: { | ||
/** The viewport to use */ | ||
viewport?: Viewport; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original motivation for this PR is to improve the types, so I would avoid changing the implementation. Adding type casts or better types is great, but you should avoid anything that will modify the behavior at runtime as there is a risk of introducing bugs