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

Add bearing to markers #8836

Merged
merged 20 commits into from
Oct 29, 2019
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 85 additions & 2 deletions src/ui/marker.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,10 @@ type Options = {
offset?: PointLike,
anchor?: Anchor,
color?: string,
draggable?: boolean
draggable?: boolean,
rotation?: number,
rotationAlignment?: string,
pitchAlignment?: string
};

/**
Expand All @@ -31,6 +34,9 @@ type Options = {
* @param {PointLike} [options.offset] The offset in pixels as a {@link PointLike} object to apply relative to the element's center. Negatives indicate left and up.
* @param {string} [options.color='#3FB1CE'] The color to use for the default marker if options.element is not provided. The default is light blue.
* @param {boolean} [options.draggable=false] A boolean indicating whether or not a marker is able to be dragged to a new position on the map.
* @param {number} [options.rotation=0] The rotation angle of the marker in degrees, relative to its respective {@link Marker#rotationAlignment} setting. A positive value will rotate the marker clockwise.
* @param {string} [options.pitchAlignment='auto'] `map` aligns the `Marker` to the plane of the map. `viewport` aligns the `Marker` to the plane of the viewport. `auto` automatically matches the value of `rotationAlignment`.
* @param {string} [options.rotationAlignment='auto'] `map` aligns the `Marker`'s rotation relative to the map, maintaining a bearing as the map rotates. `viewport` aligns the `Marker`'s rotation relative to the viewport, agnostic to map rotations. `auto` is equivalent to `viewport`.
* @example
* var marker = new mapboxgl.Marker()
* .setLngLat([30.5, 50.5])
Expand All @@ -51,6 +57,9 @@ export default class Marker extends Evented {
_draggable: boolean;
_state: 'inactive' | 'pending' | 'active'; // used for handling drag events
_positionDelta: ?number;
_rotation: number;
_pitchAlignment: string;
_rotationAlignment: string;

constructor(options?: Options, legacyOptions?: Options) {
super();
Expand All @@ -72,6 +81,9 @@ export default class Marker extends Evented {
this._color = options && options.color || '#3FB1CE';
this._draggable = options && options.draggable || false;
this._state = 'inactive';
this._rotation = options && options.rotation || 0;
this._rotationAlignment = options && options.rotationAlignment || 'auto';
this._pitchAlignment = options && options.pitchAlignment || this._rotationAlignment;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wouldn't match pitchAlignment to rotationAlignment if pitchAlignment = 'auto' and rotationAlignment = 'map'. This should be options && options.pitchAlignment && options.pitchAlignment === 'auto' ? this._rotationAlignment : options.pitchAlignment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.


if (!options || !options.element) {
this._defaultMarker = true;
Expand Down Expand Up @@ -344,14 +356,28 @@ export default class Marker extends Evented {

this._pos = this._map.project(this._lngLat)._add(this._offset);

let rotation = "";
if (this._rotationAlignment === "viewport" || this._rotationAlignment === "auto") {
rotation = `rotateZ(${this._rotation}deg)`;
} else if (this._rotationAlignment === "map") {
rotation = `rotateZ(${this._rotation - this._map.getBearing()}deg)`;
}

let pitch = "";
if (this._pitchAlignment === "viewport" || this._pitchAlignment === "auto") {
pitch = "rotateX(0deg)";
} else if (this._pitchAlignment === "map") {
pitch = `rotateX(${this._map.getPitch()}deg)`;
}

// because rounding the coordinates at every `move` event causes stuttered zooming
// we only round them when _update is called with `moveend` or when its called with
// no arguments (when the Marker is initialized or Marker#setLngLat is invoked).
if (!e || e.type === "moveend") {
this._pos = this._pos.round();
}

DOM.setTransform(this._element, `${anchorTranslate[this._anchor]} translate(${this._pos.x}px, ${this._pos.y}px)`);
DOM.setTransform(this._element, `${anchorTranslate[this._anchor]} translate(${this._pos.x}px, ${this._pos.y}px) ${pitch} ${rotation}`);
}

/**
Expand Down Expand Up @@ -484,4 +510,61 @@ export default class Marker extends Evented {
isDraggable() {
return this._draggable;
}

/**
* Sets the `rotation` property of the marker.
* @param {number} [rotation=0] The rotation angle of the marker (clockwise, in degrees), relative to its respective {@link Marker#rotationAlignment} setting.
* @returns {Marker} `this`
*/
setRotation(rotation: number) {
this._rotation = rotation || 0;
this._update();
return this;
}

/**
* Returns the current rotation angle of the marker (in degrees).
* @returns {number}
*/
getRotation() {
return this._rotation;
}

/**
* Sets the `rotationAlignment` property of the marker.
* @param {string} [alignment='auto'] Sets the `rotationAlignment` property of the marker.
* @returns {Marker} `this`
*/
setRotationAlignment(alignment: string) {
this._rotationAlignment = alignment || 'auto';
this._update();
return this;
}

/**
* Returns the current `rotationAlignment` property of the marker.
* @returns {string}
*/
getRotationAlignment() {
return this._rotationAlignment;
}

/**
* Sets the `pitchAlignment` property of the marker.
* @param {string} [alignment='auto'] Sets the `pitchAlignment` property of the marker.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't actually default to auto. I think we should remove the default here and make a note that if alignment is 'auto', it will automatically match rotationAlignment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the wording on that.

* @returns {Marker} `this`
*/
setPitchAlignment(alignment: string) {
this._pitchAlignment = alignment || this._rotationAlignment;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should also be alignment === 'auto' ? this._rotationAlignment : alignment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did this:

this._pitchAlignment = alignment && alignment !== 'auto' ? alignment : this._rotationAlignment;

To also check if it's undefined.

this._update();
return this;
}

/**
* Returns the current `pitchAlignment` property of the marker.
* @returns {string}
*/
getPitchAlignment() {
return this._pitchAlignment;
}
}
66 changes: 66 additions & 0 deletions test/unit/ui/marker.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -465,3 +465,69 @@ test('Marker with draggable:true does not error if removed on mousedown', (t) =>
t.ok(map.fire('mouseup'));
t.end();
});

test('Marker can set rotationAlignment and pitchAlignment', (t) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably add a test that ensures setting rotationAlignment === 'map' and pitchAlignment === 'auto' results in pitchAlignment === 'map'. Both through the options object and by calling set{Rotation/Pitch}Alignment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a couple tests. One is for the option on initialization, the other is for the getter/setters.

const map = createMap(t);
const marker = new Marker({rotationAlignment: 'map', pitchAlignment: 'map'})
.setLngLat([0, 0])
.addTo(map);

t.equal(marker.getRotationAlignment(), 'map');
t.equal(marker.getPitchAlignment(), 'map');

map.remove();
t.end();
});

test('Marker can set and update rotation', (t) => {
const map = createMap(t);
const marker = new Marker({rotation: 45})
.setLngLat([0, 0])
.addTo(map);

t.equal(marker.getRotation(), 45);

marker.setRotation(90);
t.equal(marker.getRotation(), 90);

map.remove();
t.end();
});

test('Marker transforms rotation with the map', (t) => {
const map = createMap(t);
const marker = new Marker({rotationAlignment: 'map'})
.setLngLat([0, 0])
.addTo(map);

const rotationRegex = /rotateZ\(-?([0-9]+)deg\)/;
const initialRotation = marker.getElement().style.transform.match(rotationRegex)[1];

map.setBearing(map.getBearing() + 180);

const finalRotation = marker.getElement().style.transform.match(rotationRegex)[1];
t.notEqual(initialRotation, finalRotation);

map.remove();
t.end();
});

test('Marker transforms pitch with the map', (t) => {
const map = createMap(t);
const marker = new Marker({pitchAlignment: 'map'})
.setLngLat([0, 0])
.addTo(map);

map.setPitch(0);

const rotationRegex = /rotateX\(-?([0-9]+)deg\)/;
const initialPitch = marker.getElement().style.transform.match(rotationRegex)[1];

map.setPitch(45);

const finalPitch = marker.getElement().style.transform.match(rotationRegex)[1];
t.notEqual(initialPitch, finalPitch);

map.remove();
t.end();
});