SceneView / sceneview-android

SceneView is a 3D and AR Android Composable and View with Google Filament and ARCore. This is a Sceneform replacement in Kotlin
Apache License 2.0
756 stars 151 forks source link

fix: Camera.viewToRay end position calculation #483

Closed kubax2000 closed 1 month ago

kubax2000 commented 1 month ago

400 fix

If z equals 1.0f viewToWorld returns [NaN, NaN, -Infinity].

grassydragon commented 1 month ago

Hi! Are you sure this pull request fixes the referenced issue?

ThomasGorisse commented 1 month ago

You're right @grassydragon. The issue might be coming from there: https://github.com/SceneView/sceneview-android/blob/main/sceneview%2Fsrc%2Fmain%2Fjava%2Fio%2Fgithub%2Fsceneview%2Futils%2FCamera.kt#L253-L267 @grassydragon This code was coming from Sceneform but I can't figure out what is wrong. Could you have a quick look at it?

grassydragon commented 1 month ago

Could you have a quick look at it?

Yes, I'll take a look today in the evening.

grassydragon commented 1 month ago

In Sceneform there is a return then when w is close to 0 and z is set to 0 to in this case: https://github.com/SceneView/sceneform-android/blob/master/core/src/main/java/com/google/ar/sceneform/Camera.java#L386-L389

ThomasGorisse commented 1 month ago

That's also what I had in mind but do you think the NAN could come from there?

ThomasGorisse commented 1 month ago

Here is the fix I had in mind 😀 https://www.linkedin.com/posts/thomas-gorisse_android-studio-gemini-integration-is-simply-activity-7198315922854490112-2c3E?utm_source=share&utm_medium=member_android

den59k commented 1 month ago

Hi! I faced the same problem - screen tap didn't work. I found out that it was due to incorrect operation of * operator from dev.romainguy.kotlin.math library.

To be sure, replace the line https://github.com/SceneView/sceneview-android/blob/ffb2f40500c6d1807d33a028e0a50f9f6630a79f/sceneview/src/main/java/io/github/sceneview/utils/Camera.kt#L261

with

    val arr = inverse(projectionTransform * viewTransform).toFloatArray()
    val vec = clipSpacePosition.toFloatArray()
    val resultVec = FloatArray(4)
    Matrix.multiplyMV(resultVec, 0, arr, 0, vec, 0)
    val result = resultVec.toFloat4()

and get the correct answer

I hope this helps in finding the right solution

grassydragon commented 1 month ago

That's also what I had in mind but do you think the NAN could come from there?

@ThomasGorisse, to be honest, I don't understand the formula in the unproject method completely. I'll see how it's implemented in other libraries.

ThomasGorisse commented 1 month ago

Thanks a lot @den59k . That was very very tricky to find. Good job!!! @grassydragon Don't you think it's only due to the usual row/column order inversion between kotlin-math and Filament?

grassydragon commented 1 month ago

Don't you think it's only due to the usual row/column order inversion between kotlin-math and Filament?

I'll try soon and tell the results. I checked yesterday that other libraries, for example, GLM, had the same formula for unprojecting a point so the problem isn't in the formula.

den59k commented 1 month ago

Yeah, it doesn't seem to be the formula, sorry. I found out after I started testing further.

But my click started working correctly after I replaced projectionTransform with cullingProjectionTransform

@ThomasGorisse I will describe the problem of why NaN occurs:

When we project a point onto projectionTransform, the w component is zero (the point goes to infinity). Then there is a condition that replaces xy by 0 in this case (I don't know why this is here, it looks like a dirty hack). And at the end the result is multiplied by 1/w, and in the Kotlin 0 * Infinity = NaN

grassydragon commented 1 month ago

Don't you think it's only due to the usual row/column order inversion between kotlin-math and Filament?

@ThomasGorisse, no, I've checked and this isn't the case here.

But my click started working correctly after I replaced projectionTransform with cullingProjectionTransform

Yes, @den59k is absolutely right. We can't use the projectionTransform since the Filament documentation says that this projection matrix has its far plane set to infinity.

I think the final code of the method should be as follows:

fun Camera.viewToWorld(viewPosition: Float2, z: Float = 1.0f): Position {
    // Normalize between -1 and 1.
    val clipSpacePosition = Float4(
        x = viewPosition.x * 2.0f - 1.0f,
        y = viewPosition.y * 2.0f - 1.0f,
        z = 2.0f * z - 1.0f,
        w = 1.0f
    )
    val result = inverse(cullingProjectionTransform * viewTransform) * clipSpacePosition

    if (MathHelper.almostEqualRelativeAndAbs(result.w, 0.0f)) {
        return Position()
    }

    return result.xyz / result.w
}
ThomasGorisse commented 1 month ago

Great work here! I'll create the PR with a MathHelper.almostEqualRelativeAndAbs() replacement applied to Kotlin-math and check if everything is alright. Thanks all.

ThomasGorisse commented 1 month ago

Fixed in 2.2.0 from https://github.com/SceneView/sceneview-android/commit/5cc1fb049433bbad48d47cc7240320bc8bb07a1b