-
Notifications
You must be signed in to change notification settings - Fork 157
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
Example: use SceneKit in custom layer #154
Conversation
transformSimd[3, 2] = m[14].doubleValue | ||
transformSimd[3, 3] = m[15].doubleValue | ||
|
||
let origin = try! Projection.project(for: modelOrigin, zoomScale: 1.0 / 512.0); |
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.
What's the 512 here?
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.
Explained in code comment:
// Projection.project(for: modelOrigin, zoomScale: 1.0 / 512.0) corresponds to gl-js's
// mapboxgl.MercatorCoordinate.fromLngLat(). origin is in spherical mercator normalized to
// 0..1 for the width of the world. In other words, (x,y) E [0..1) is used to represent
// coordinates in one world copy, values of x +/- 1 represent wrap.
let origin = try! Projection.project(for: modelOrigin, zoomScale: 1.0 / 512.0)
public func render(_ parameters: CustomLayerRenderParameters, mtlCommandBuffer: MTLCommandBuffer, mtlRenderPassDescriptor: MTLRenderPassDescriptor) { | ||
let m = parameters.projectionMatrix; | ||
|
||
// It is essential to use double precision for computation below: using simd instead |
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.
Can you say why - presumably because of jittering with floats?
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.
Yes, mercator coordinate addresses whole world and there's just not enough precision if using 32-bit floating point computation.
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.
Can you add that to the comment please?
if #available(iOS 13.0, *) { | ||
renderer.usesReverseZ = false | ||
} else { | ||
// Fallback on earlier versions, disable depth in render() |
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.
What happens in these cases? Should we disable this example completely if < iOS 13?
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.
We should enable it, just there is a need to use CPU side occlusion (the same approach that could be used when geo-referencing UI elements to map). The idea is to disable depth buffer (now disabled in demo) and show or hide model with animated fade in / fade out (similar to behavior of markers in gl-js).
As we cannot have occlusion done yet, current behavior on iOS < 13 is like here (model always visible):
Screen.Recording.2021-03-03.at.19.28.43.mov
There was a lag/delay when panning the map. projectionTransform property is animatable and it is need to call SCNTransaction.flush() to use it in current frame. Updated video shows improved behavior. @julianrex PTAL |
override public func viewDidAppear(_ animated: Bool) { | ||
super.viewDidAppear(animated) | ||
// The below line is used for internal testing purposes only. | ||
self.finish() |
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.
Let's move this to renderingWillEnd
(and rebase on main once #158 lands). These examples get used as tests, and we want to ensure that we render the custom layer.
Can you make sure this example runs as a test (Cmd-U on the Examples target should be sufficient).
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.
Done. Test passes. However, as it is the case with this and several other tests, I could see
Example timed out, was this intentional? Call finish() if possible.
Test Case '-[ExamplesTests.TestableExampleTests testSceneKitExample]' passed (20.534 seconds).
let insertHillshadeBelow = try! map.styleLayerExists(forLayerId: "water") ? | ||
LayerPosition(above: nil, below: "water", at: nil) : try! map.styleLayerExists(forLayerId: "hillshade") ? | ||
LayerPosition(above: nil, below: "hillshade", at: nil) : nil |
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.
This is a little hard to understand; can we split this into multiple lines.
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.
Unnecessary complication - replaced the code with simple LayerPosition(above: nil, below: "water", at: nil)
as it is present in style used.
let modelScale = makeScaleMatrix(xScale: meterInMercatorCoordinateUnits, yScale: -meterInMercatorCoordinateUnits, zScale: meterInMercatorCoordinateUnits) | ||
|
||
// mercator scale is specific to gl-native example because gl-js's customLayerMatrix computes this | ||
// internaly: https://github.com/mapbox/mapbox-gl-js/blob/main/src/geo/transform.js#L1316 |
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.
nit: internally.
// mapboxgl.MercatorCoordinate.fromLngLat(). origin is in spherical mercator normalized to | ||
// 0..1 for the width of the world. In other words, (x,y) E [0..1) is used to represent | ||
// coordinates in one world copy, values of x +/- 1 represent wrap. | ||
let origin = try! Projection.project(for: modelOrigin, zoomScale: 1.0 / 512.0) |
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.
I still don't understand what the 512 represents here and in the subsequent code; shouldn't the projection calculation here involve parameters.zoom
?
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.
This is a good point. I have apparently approached this from wrong angle; the idea was to complicate matrices and calculation, enhance the complexity with differences between gl-js and gl-native custom layer transformation matrix and Web mercator scaling internals, in order to present 1-1 mapping of gl-js example to approach here. It is wrong and if using parameters.zoom this is much simpler to follow (and less code).
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.
This is super exciting to see. The simd usage significantly improves upon the approach in mapbox/ios-sdk-examples#111 (comment).
It would be wonderful to see MapboxMaps eventually encapsulate SceneKit integration to a greater degree – perhaps even as a “SceneKitLayer” class. There are a lot of transformations and settings here that would be common to most usage of SceneKit. I think developers would likely copy-paste the whole example rather than take advantage of any flexibility gained by implementing it at the application level.
73f8a45
to
5c204bc
Compare
@julianrex PTAL |
5c204bc
to
85b813d
Compare
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.
A few comments that would be good to address, but otherwise LGTM. Approved so you're not blocked.
|
||
var demSource = RasterDemSource() | ||
demSource.url = "mapbox://mapbox.mapbox-terrain-dem-v1" | ||
demSource.tileSize = 514 |
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.
Is 514 correct?
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.
514 is used for padded (border ready) DEM tiles. I wanted to test it here and left the value.
Padded DEM tiles are performance improvement, especially for hillshade: borders are prepared on server side and there is no need to backfill borders from neighboring DEM tiles.
Anyway, it is unnecessary, additional information for this PR. I'll remove before merging.
public func render(_ parameters: CustomLayerRenderParameters, mtlCommandBuffer: MTLCommandBuffer, mtlRenderPassDescriptor: MTLRenderPassDescriptor) { | ||
let m = parameters.projectionMatrix; | ||
|
||
// It is essential to use double precision for computation below: using simd instead |
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.
Can you add that to the comment please?
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.
Thank you for the PR!
Port "antenna building" example from gl-js https://docs.mapbox.com/mapbox-gl-js/example/add-3d-model/
85b813d
to
8832550
Compare
hey @astojilj thanks for this great example. Im looking at using something similar to this with models on a map. Do you have any idea how to respond to a touch on one of these models? I am trying out the hittest method on the renderer but having no luck unless you are directly above it and close. |
@astojilj Is there an example for this on Android SDK? |
@Cal-um,
for hit testing. If you have time to experiment with it, please try, I will also allocate some time later this week to check it. |
@NickLambert95 , |
@astojilj I just replied to your comment, it seems you closed the issue thread so I just wanted to let you know |
thanks @astojilj Ill have a play around with it. The use case I have is displaying multiple models (15+) on the map similar to the satellite dish in your example so they could be a range of shapes and sizes. I know this may complicate things but I'm getting an idea of what kind of challenge this would be to identify taps on all of them. |
Hey @astojilj Sorry for pestering you on this but I am a complete noob at this. Perhaps I am approaching this wrong but I have tried taking the CGPoint of the tap and unprojecting it from the renderer method with a SCNVector3 of z 0 and 1. It seems to work when rendering a line with it but it's at completely the wrong point on the map. On a different note would you need a new scene and layer for every distinct model rendered on the map? Thanks in advance. |
Port "antenna building" example from gl-js https://docs.mapbox.com/mapbox-gl-js/example/add-3d-model/
Screen.Recording.2021-03-17.at.14.34.49.mov
The example demonstrates direct port from https://docs.mapbox.com/mapbox-gl-js/example/add-3d-model/ using Three.js to SceneKit.
For that purpose, original GLTF model is converted (using Blender) to dae. Textures are embedded in attached dae file. When exporting in Blender, it is important to set with Y-up in export setting. Original model is available from https://nasa3d.arc.nasa.gov/detail/jpl-vtad-dsn34.
Code comments provide in depth explanation on matrices transformations required to geo-reference models on map.
Initial version used SceneKit SCNMatrix4 operations for positioning model on the map. That led to visible jittering of model relative to camera. Similar issue was observed using simd floating point calculations. The jittering looked like this:
Screen.Recording.2021-03-17.at.14.24.23.mov
To resolve that, it is necessary to use simd double precision calculation for following:
mapbox-maps-ios/Examples/Examples/All Examples/SceneKitExample.swift
Lines 177 to 187 in 9804c12
Issues:
Follow up: After #155 , implement CPU side occlusion as an option.