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

Fix for queryRenderedFeatures on fill-extrusions where a large part of the extrusion is behind the camera #10428

Merged
merged 2 commits into from
Mar 4, 2021

Conversation

ivovandongen
Copy link
Contributor

For fill extrusions with parts behind the camera the projection was wrong resulting in false positives for query rendered features.

Before:
before_clamping

After:
after_clamping

Launch Checklist

  • briefly describe the changes in this PR
  • include before/after visuals or gifs if this PR includes visual changes
  • write tests for all new functionality
  • manually test the debug page
  • tagged @mapbox/gl-native if this PR includes shader changes or needs a native port
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
  • add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog>Fix queryRenderedFeatures for fill-extrusions partly behind the camera</changelog>

out[2] = (m[2] * x + m[6] * y + m[10] * z + m[14]) / w;
return out;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need a specialized transform function for this purpose. You could use a generic matrix multiplication instead (m44 * m41) and divide the resulting vector by w afterwards. Something like this:

const vec = mul(m, [a0, a1, a2, 1]);
vec /= max(vec[3], 0.00001);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 4519789

Copy link
Contributor

@mpulkki-mapbox mpulkki-mapbox left a comment

Choose a reason for hiding this comment

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

Clamping the w component looks good and should be sufficient to cover this corner case!

@@ -232,12 +231,12 @@ function projectExtrusion2D(geometry: Array<Array<Point>>, zBase: number, zTop:
const baseX = sX + baseXZ;
const baseY = sY + baseYZ;
const baseZ = sZ + baseZZ;
const baseW = sW + baseWZ;
const baseW = Math.max(sW + baseWZ, 0.00001);
Copy link
Contributor

@rreusser rreusser Mar 3, 2021

Choose a reason for hiding this comment

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

A comment or link to this issue might be helpful since the reason for this line may not be obvious.

Also, can baseW be negative but not close to zero? Just checking since it's not clear to me what this would compute if that were the case. For example, to avoid zero without otherwise changing the magnitude or sign of the number: Math.sign(sW + baseWZ) * Math.max(Math.abs(sW + baseWZ), 0.00001);.

Copy link
Contributor

@rreusser rreusser Mar 3, 2021

Choose a reason for hiding this comment

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

(To at least be clear if I'm going to be pedantic, because it's homogeneous, the result would be exactly identical if all of the components were simply negated—unless the clamping didn't permit negative w in an exactly equivalent vector. Just wanted to be certain that it's an upstream guarantee that w is non-negative.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A comment or link to this issue might be helpful since the reason for this line may not be obvious.

There is no issue for this. Discovered it while porting various QRF pr's to gl-native and needed a test, so also fixed it here. The PR and query tests should serve as plenty of documentation.

Also, can baseW be negative but not close to zero?

Yes, that is actually a major part of the issue

For example, to avoid zero without otherwise changing the magnitude or sign of the number

It's basically clamping, which serves us well in this case as we don't need the magnitude when w < 0.

@karimnaaji karimnaaji added this to the v2.2 milestone Mar 3, 2021
@ivovandongen ivovandongen merged commit c4229dc into main Mar 4, 2021
@ivovandongen ivovandongen deleted the ivd_fix_fe_qrf branch March 4, 2021 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants