[ios, macos] fixes #6160: allow updating multipoint coordinates #6565

incanus · 2016-10-03T23:25:02Z

This adds -[MGLMultiPoint updateCoordinates:count:] and pipes to core, which immediately updates display of MGLPolyline or MGLPolygon annotations.

Several design considerations:

Went with updateCoordinates:... naming since merely making the coordinates property readwrite (effectively) as an analog to -[MGLPointAnnotation coordinate] is not doable with C arrays of CLLocationCoordinate2D structs.
Was a little duplicative with internal comments and deselection in MGLMapView for the sake of clarity and safety.
Tried to keep free and malloc together in scope for future maintenance safety.

/cc @1ec5 @friedbunny @boundsj

mention-bot · 2016-10-03T23:25:07Z

@incanus, thanks for your PR! By analyzing the history of the files in this pull request, we identified @jfirebaugh, @1ec5 and @boundsj to be potential reviewers.

incanus · 2016-10-03T23:25:31Z

You can test this with a small patch such as the following:

https://gist.github.com/incanus/fb2ffe1b235c9a132cd9d51dcfb369dd

1ec5 · 2016-10-03T23:45:49Z

For consistency with -getCoordinates:inRange:, the new method should be called -setCoordinates:inRange:. This allows the developer to update the polyline or polygon piecemeal.

incanus · 2016-10-03T23:49:17Z

Going to implement @1ec5's suggestion; this is better API.

incanus · 2016-10-03T23:50:05Z

What about also having just -setCoordinates:count:for simplicity's sake? A count is easier to form than a range.

1ec5 · 2016-10-03T23:48:52Z

platform/darwin/src/MGLMultiPoint.mm

+    if ([self isMemberOfClass:[MGLMultiPoint class]])
+    {
+        [[NSException exceptionWithName:@"MGLAbstractClassException"
+                                 reason:@"MGLMultiPoint is an abstract class"


MGLMultiPoint is no longer technically an abstract class, since we're overloading it to represent the GeoJSON MultiPoint concept as well. The exception here is a good idea, but we should move the declaration of this method to MGLPolyline and MGLPolygon individually.

@1ec5 Do you have more context on this? I'm not sure I understand the GeoJSON concept you are referring to.

For the purposes of feature querying (but not annotations yet), we use this class to represent GeoJSON MultiPoints, which are just collections of points with no vertices connecting them. #5201 tracks cleaning up the class hierarchy so that MGLPolyline and MGLPolygon no longer inherit from MGLMultiPoint.

👉 #5201 (comment)

Well, no, once #5201 lands, this exception and the others like it won’t even be necessary anymore. But in the meantime, all I’m suggesting is that we say:

[NSException raise:@"Unsupported method" format:@"The coordinates of a member of MGLMultiPoint are read-only."];

I think I finally follow @1ec5; adjusted in 8d92589.

1ec5 · 2016-10-03T23:49:45Z

platform/darwin/src/MGLMultiPoint.mm

+                               userInfo:nil] raise];
+    }
+
+    assert(_count > 0);


For internal errors in Objective-C code, prefer NSAssert over a C++ assert. In this case, we're catching a developer error, so it should be a full-blown NSException.

Was following the pattern in -getCoordinates:range::

assert(range.location + range.length <= _count);

OK, I guess we missed that too when we overloaded this class to represent multipoints.

1ec5 · 2016-10-03T23:52:50Z

What about also having just -setCoordinates:count:for simplicity's sake? A count is easier to form than a range.

With a range, the developer could potentially append to the polyline without having to rebuild the whole polyline from scratch.

Having both methods complicates the code for little gain, I think. It would optimize for the case where the developer needs to modify the beginning n coordinates frequently, which is probably a minor use case compared to modifying something in the middle or appending to the polyline. It's easy enough to call NSMakeRange with a first parameter of 0, if that's what you want.

incanus · 2016-10-03T23:56:11Z

Sorry, I mean in addition:

- (void)setCoordinates:(CLLocationCoordinate2D *)coords range:(NSRange)range;
- (void)setCoordinates:(CLLocationCoordinate2D *)coords count:(NSUInteger)count;

friedbunny · 2016-10-03T23:59:27Z

@incanus Please also add a changelog entry. I imagine we should create a port-to-macOS ticket as well, for API additions there.

1ec5 · 2016-10-04T00:27:27Z

platform/ios/src/MGLMapView.mm

@@ -2840,6 +2863,8 @@ - (void)addAnnotations:(NS_ARRAY_OF(id <MGLAnnotation>) *)annotations
            MGLAnnotationContext context;
            context.annotation = annotation;
            _annotationContextsByAnnotationTag[annotationTag] = context;
+
+            [(NSObject *)annotation addObserver:self forKeyPath:@"coordinates" options:0 context:(void *)(NSUInteger)annotationTag];


For MGLPolygon, also observe changes to interiorPolygons.

Although this could be tail work, since it would require making the interiorPolygons property mutable or at least settable.

incanus · 2016-10-04T17:49:53Z

Ok, range-based updating works. Here's constantly adding the map center to a polyline as the map moves:

…multipoints

incanus · 2016-10-04T17:56:17Z

Updated testing patch:

https://gist.github.com/incanus/d84f8079660458ff4df7865d4ba64326

incanus · 2016-10-04T19:10:23Z

macOS is set as of 0349363. Test patch here:

https://gist.github.com/incanus/2f6dac071f2a744ae8bf55fa35ab454a

…multipoints

incanus · 2016-10-04T22:00:33Z

Ok, lots of work today and ready for review again. Let's 🚢this 🦃.

1ec5 · 2016-10-04T22:18:40Z

platform/darwin/src/MGLMultiPoint.mm

@@ -93,6 +84,47 @@ - (void)getCoordinates:(CLLocationCoordinate2D *)coords range:(NSRange)range
    }
 }

+- (void)setCoordinates:(CLLocationCoordinate2D *)coords range:(NSRange)range
+{
+    if ([self isMemberOfClass:[MGLMultiPoint class]])


Oof, I totally forgot this method is defined on MGLMultiPoint rather than MGLPolyline and MGLPolygon individually. In that case, this check should go away too, because there’s no reason you couldn’t get and set the coordinates of an MGLMultiPoint instance.

1ec5 · 2016-10-04T22:19:38Z

platform/darwin/src/MGLMultiPoint.mm

+
+    if (range.length == 0)
+    {
+        [[NSException exceptionWithName:NSRangeException


This would be more concise with a call to +[NSException raise:format:].

1ec5 · 2016-10-04T22:21:20Z

platform/darwin/src/MGLMultiPoint.mm

+        // replacing existing coordinate(s)
+        memcpy(_coordinates + range.location, coords, range.length * sizeof(CLLocationCoordinate2D));
+    }
+    else if (NSMaxRange(range) > _count)


Nit: this can just be else.

1ec5 · 2016-10-04T22:25:51Z

platform/darwin/src/MGLMultiPoint.mm

+                               userInfo:nil] raise];
+    }
+
+    [self willChangeValueForKey:@"coordinates"];


pointCount can also change. Might be worth implementing +keyPathsForValuesAffectingPointCount:.

1ec5 · 2016-10-04T22:29:34Z

platform/darwin/src/MGLMultiPoint.mm

+        // appending new coordinate(s)
+        NSUInteger newCount = NSMaxRange(range);
+        CLLocationCoordinate2D *newCoordinates = (CLLocationCoordinate2D *)malloc(newCount * sizeof(CLLocationCoordinate2D));
+        memcpy(newCoordinates, _coordinates, _count * sizeof(CLLocationCoordinate2D));


If the developer is overwriting the tail end of the array while also extending it, we should avoid unnecessary work: take the minimum of _count and range.location.

1ec5 · 2016-10-04T22:29:41Z

platform/darwin/src/MGLMultiPoint.mm

+        // appending new coordinate(s)
+        NSUInteger newCount = NSMaxRange(range);
+        CLLocationCoordinate2D *newCoordinates = (CLLocationCoordinate2D *)malloc(newCount * sizeof(CLLocationCoordinate2D));
+        memcpy(newCoordinates, _coordinates, _count * sizeof(CLLocationCoordinate2D));


Now that the address that the coordinates property points to can change, we need a warning on that property’s documentation along the lines of -[NSString UTF8String] (another example of an NS_RETURNS_INNER_POINTER method).

1ec5 · 2016-10-04T22:36:18Z

platform/darwin/src/MGLMultiPoint.mm

+    if (NSMaxRange(range) <= _count)
+    {
+        // replacing existing coordinate(s)
+        memcpy(_coordinates + range.location, coords, range.length * sizeof(CLLocationCoordinate2D));


memcpy() documentation states that the destination and source of a copy mustn’t overlap, or Bad Things™ can happen. So we should avoid calling memcpy() in the case that either coords or coords + coords.length falls between _coordinates + range.location and _coordinates + NSMaxRange(range). Since this is a bit of an edge case – you’d have to pass in the coordinates property as coords – it’s fine to punt on that by raising an exception, but we should ticket it out as tail work.

I'll consider this handled by the #6580 refactor and the fact that fcd5ada says yet as a cue.

This will catch the case in which the developer tries to close a polyline:

[polyline setCoordinates:polyline.coordinates range:NSMakeRange(polyline.count, 1)];

but not if they try to have it return to the midpoint:

[polyline setCoordinates:polyline.coordinates + polyline.pointCount / 2 range:NSMakeRange(polyline.count, 1)];

I think the check should be expanded to something like:

(coords >= _coordinates + range.location && coords < pointCount + NSMaxRange(range)) || (coords + range.length >= pointCount + range.location && coords + range.length < pointCount + NSMaxRange(range))

By the way, in that case, it would be safe to call memmove() instead of memcpy().

Will do. I'm going to avoid memmove() right now since it'll go away with the refactor.

I think I got this ok in 7f6049b?

I think this check may be off by 1 since _count is not 0 indexed and, when added to the array, it points at memory somewhere off the end of the array.

Actually, the comparison is < so it is probably fine. Sorry!

1ec5 · 2016-10-04T22:53:40Z

platform/darwin/src/MGLMultiPoint.h

+ and/or a `length` which extends past the end of the existing array. The array 
+ in _`coords`_ must be equal in number to the length of the range.
+ */
+- (void)setCoordinates:(CLLocationCoordinate2D *)coords range:(NSRange)range;


Something didn’t feel right to me, and now I know why: we’re only allowing the developer to overwrite or extend the multipoint but not shrink it. They can splice in but not splice out. We’re assuming that coords’ range is equal to range.length, whereas the length would have to be passed in explicitly.

I don’t think we should take the time right now to implement all the possible splicing operations, but we should try to be consistent with naming convention for KVO indexed accessors, which NSMutableArray and others adhere to. Ideally, we’d call this method -replaceCoordinatesInRange:withCoordinates:count: and have separate -setCoordinates:count, -insertCoordinates:count:atIndex:, and -removeCoordinatesInRange: methods. But for now, let’s call this method -replaceCoordinatesInRange:withCoordinates:. When we eventually implement -replaceCoordinatesInRange:withCoordinates:count:, we can deprecate this method.

1ec5 · 2016-10-04T23:13:12Z

#6580 would eliminate the manual C memory management and make it a lot easier (think one-liners) to implement the rest of the operations laid out in #6565 (comment).

incanus · 2016-10-04T23:47:24Z

Again all good to go. 😅

1ec5 · 2016-10-05T03:42:43Z

Thanks for the great work here @incanus! I’ve filed #6580 for the other half of this feature.

nitrag · 2016-11-07T01:26:41Z

Need guidance...I'm trying to add/update a Polyline on the fly.

var recordLine: MGLPolyline? = nil
func updateRecordingLine(_ coords: [CLLocationCoordinate2D]){
    var recordLineCoords = coords
    if(self.recordLine == nil){
        //add first time
        self.recordLine = MGLPolyline(coordinates: &recordLineCoords, count: UInt(recordLineCoords.count))
        self.mapView.addAnnotation(self.recordLine!)
    }else{
        //update
        self.recordLine?.replaceCoordinates(in: NSMakeRange(0, recordLineCoords.count), withCoordinates: &recordLineCoords)
    }
}

JesseCrocker · 2016-11-11T18:07:42Z

Not being able to update a line with fewer coordinates than it previously had seems like an odd limitation.

incanus · 2016-11-11T18:20:12Z

@JesseCrocker 👉 #6583

[ios, macos] fixes #6160: allow updating multipoint coordinates

086297d

incanus added feature iOS Mapbox Maps SDK for iOS performance Speed, stability, CPU usage, memory usage, or power usage macOS Mapbox Maps SDK for macOS annotations Annotations on iOS and macOS or markers on Android beta blocker Blocks the next beta release labels Oct 3, 2016

incanus added this to the ios-v3.4.0 milestone Oct 3, 2016

incanus self-assigned this Oct 3, 2016

incanus mentioned this pull request Oct 3, 2016

MGLPolygon, MGLPolyline coordinates should be mutable #6160

Closed

incanus added the ✓ ready for review label Oct 3, 2016

boundsj approved these changes Oct 3, 2016

View reviewed changes

1ec5 suggested changes Oct 3, 2016

View reviewed changes

1ec5 suggested changes Oct 4, 2016

View reviewed changes

incanus added 2 commits October 4, 2016 10:53

move to range-based point updating

156071a

Merge remote-tracking branch 'origin/master' into 6160-update-darwin-…

d85aec1

…multipoints

assert() -> NSAssert() for user errors

e7888e1

incanus mentioned this pull request Oct 4, 2016

allow mutation of MGLPolygon interior polygons/holes #6576

Closed

macos support for polyline/polygon coordinates mutation

0349363

incanus added 5 commits October 4, 2016 14:04

Merge remote-tracking branch 'origin/master' into 6160-update-darwin-…

6e61b93

…multipoints

clarify changelog

608115e

NSAssert() -> NSException for external uses

6e607ec

simplify operations with memcpy() & avoid for loops

2d46f20

adjust exceptions for MGLMultiPoint modernity

8d92589

1ec5 suggested changes Oct 4, 2016

View reviewed changes

incanus added 3 commits October 4, 2016 16:01

remove unneeded use check

a2377ed

formatting & brevity

d240ba9

remove redundant control flow

18cd62a

1ec5 mentioned this pull request Oct 4, 2016

Use std::vector for MGLMultiPoint coordinate storage #6580

Closed

incanus added 5 commits October 4, 2016 16:16

pointCount KVO is dependent on coordinates

ea0c900

memory warning for inner pointer

b9c10e3

avoid unnecessary copying

d4cb5a3

disallow pointer reuse

fcd5ada

rename method

138ae55

better check for coordinate memory reuse

7f6049b

1ec5 approved these changes Oct 5, 2016

View reviewed changes

incanus merged commit 105673e into master Oct 5, 2016

incanus deleted the 6160-update-darwin-multipoints branch October 5, 2016 01:42

1ec5 mentioned this pull request Oct 5, 2016

MGLMultiPoint (MGLPolyline, MGLPolygon) should allow removing coordinates #6583

Closed

1ec5 removed the ✓ ready for review label Oct 5, 2016

1ec5 mentioned this pull request Oct 18, 2016

Add and modify individual features in a GeoJSON source on the fly #6177

Open

1ec5 mentioned this pull request Oct 30, 2018

Reimplement MGLAnnotation atop MGLSource and MGLStyleLayer #6181

Closed

6 tasks

[ios, macos] fixes #6160: allow updating multipoint coordinates #6565

[ios, macos] fixes #6160: allow updating multipoint coordinates #6565

Conversation

incanus commented Oct 3, 2016

mention-bot commented Oct 3, 2016

incanus commented Oct 3, 2016 • edited Loading

1ec5 commented Oct 3, 2016

incanus commented Oct 3, 2016

incanus commented Oct 3, 2016 • edited Loading

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

1ec5 Oct 4, 2016 • edited Loading

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

1ec5 commented Oct 3, 2016 • edited Loading

incanus commented Oct 3, 2016

friedbunny commented Oct 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

incanus commented Oct 4, 2016

incanus commented Oct 4, 2016 • edited Loading

incanus commented Oct 4, 2016 • edited Loading

incanus commented Oct 4, 2016

1ec5 Oct 4, 2016 • edited Loading

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

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

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

1ec5 Oct 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 commented Oct 4, 2016

incanus commented Oct 4, 2016

1ec5 commented Oct 5, 2016

nitrag commented Nov 7, 2016

JesseCrocker commented Nov 11, 2016

incanus commented Nov 11, 2016

incanus commented Oct 3, 2016 •

edited

Loading

incanus commented Oct 3, 2016 •

edited

Loading

1ec5 Oct 4, 2016 •

edited

Loading

1ec5 commented Oct 3, 2016 •

edited

Loading

incanus commented Oct 4, 2016 •

edited

Loading

incanus commented Oct 4, 2016 •

edited

Loading

1ec5 Oct 4, 2016 •

edited

Loading

1ec5 Oct 4, 2016 •

edited

Loading