Skip to content

enhance(android): nearby marker 2D and 3D type#30

Open
DharaneshSP wants to merge 1 commit intomaster-testfrom
2d_3d_changes
Open

enhance(android): nearby marker 2D and 3D type#30
DharaneshSP wants to merge 1 commit intomaster-testfrom
2d_3d_changes

Conversation

@DharaneshSP
Copy link
Copy Markdown

Does any other open PR do the same thing?

(please answer here)

What issue is this PR fixing?

(please link the issue here)

How did you test this PR?

(please answer here)

Copy link
Copy Markdown

@prakharritik prakharritik left a comment

Choose a reason for hiding this comment

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

The left/right icon split and the 2D/3D caching approach are sensible. Three concrete issues to fix.

private final Map<String, Marker> nearbyMarkersCache = new HashMap<>();
private final Map<String, Marker> nearbyMarkersCalloutCache = new HashMap<>();
private final Map<String, com.google.android.gms.maps.model.BitmapDescriptor> nearbyMarker2DIconsCache = new HashMap<>();
private final Map<String, com.google.android.gms.maps.model.BitmapDescriptor> nearbyMarker3DIconsCache = new HashMap<>();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Memory leak: icon caches are never cleared when markers are removed.

When a driver/bus marker is removed (the iterator loop at the end of updateNearbyMarkersFromProcessedData), the code cleans up markerAnimators and nearbyMarkersCalloutCache for that driverId, but not these two icon caches. BitmapDescriptor wraps a native Android Bitmap which is a large unmanaged object. Over a long session where buses continuously appear and disappear, this will cause steady memory growth.

Add the corresponding remove calls in the cleanup block alongside nearbyMarkersCalloutCache.remove(driverId):

nearbyMarker2DIconsCache.remove(driverId);
nearbyMarker3DIconsCache.remove(driverId);
nearbyMarkersCalloutCache.remove(driverId);

}
else{
existingCalloutMarker.setVisible(isVisible && !hideCallout);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Redundant setVisible call.

existingCalloutMarker.setVisible(isVisible && !hideCallout) is called unconditionally at line 2261, immediately after the existingCalloutMarker != null guard. This else branch (reached only when !forceUpdate) calls it again with exactly the same arguments — it's a no-op. Remove the duplicate:

if(existingCalloutMarker != null){
    existingCalloutMarker.setVisible(isVisible && !hideCallout);
    if(forceUpdate){
        // ... icon update logic
    }
    // no else needed — visibility already set above
}

rotation = locStart.bearingTo(locEnd);
} else {
} else if(forceUpdate == false) {
rotation = existingMarker.getRotation();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Non-idiomatic Java: forceUpdate == false should be !forceUpdate.

Comparing a boolean to false with == is valid but non-idiomatic. Use !forceUpdate instead:

} else if (!forceUpdate) {
    rotation = existingMarker.getRotation();
}

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

Successfully merging this pull request may close these issues.

2 participants