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: Camera.viewToRay end position calculation #483

Closed
wants to merge 1 commit into from

Conversation

kubax2000
Copy link

@kubax2000 kubax2000 commented May 13, 2024

#400 fix

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

@kubax2000 kubax2000 closed this May 13, 2024
@kubax2000 kubax2000 reopened this May 13, 2024
@grassydragon
Copy link
Contributor

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

@ThomasGorisse
Copy link
Contributor

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
Copy link
Contributor

Could you have a quick look at it?

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

@grassydragon
Copy link
Contributor

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
Copy link
Contributor

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

@ThomasGorisse
Copy link
Contributor

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
Copy link

den59k commented May 26, 2024

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

val result = inverse(projectionTransform * viewTransform) * clipSpacePosition

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link

den59k commented May 27, 2024

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
Copy link
Contributor

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
Copy link
Contributor

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 added a commit that referenced this pull request May 29, 2024
@ThomasGorisse
Copy link
Contributor

Fixed in 2.2.0 from 5cc1fb0

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.

None yet

4 participants