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
754 stars 151 forks source link

GestureDetector doesn't update correctly #535

Open WildOrangutan opened 4 days ago

WildOrangutan commented 4 days ago

Problem

When rememberOnGestureListener is recomposed, listeners are not updated.

Example

Please see example below.

After "option" is selected by using bottom sheet (e.g. "First"), you would expect that "Single tap confirmed (option: First)" is printed to console. Instead "Single tap confirmed (option: null)" is shown.

import android.util.Log
import androidx.compose.foundation.clickable
import androidx.compose.foundation.layout.Box
import androidx.compose.foundation.layout.fillMaxSize
import androidx.compose.foundation.layout.navigationBarsPadding
import androidx.compose.foundation.layout.padding
import androidx.compose.foundation.lazy.LazyColumn
import androidx.compose.foundation.lazy.items
import androidx.compose.material.icons.Icons
import androidx.compose.material.icons.filled.Menu
import androidx.compose.material3.Icon
import androidx.compose.material3.IconButton
import androidx.compose.material3.ModalBottomSheet
import androidx.compose.material3.SheetState
import androidx.compose.material3.Text
import androidx.compose.material3.rememberModalBottomSheetState
import androidx.compose.runtime.Composable
import androidx.compose.runtime.getValue
import androidx.compose.runtime.mutableStateOf
import androidx.compose.runtime.remember
import androidx.compose.runtime.rememberCoroutineScope
import androidx.compose.runtime.setValue
import androidx.compose.ui.Alignment
import androidx.compose.ui.Modifier
import androidx.compose.ui.unit.dp
import io.github.sceneview.ar.ARScene
import io.github.sceneview.rememberOnGestureListener
import kotlinx.coroutines.launch

private val options = listOf(
    "First",
    "Second"
)

@Composable
fun Main() {
    var selectedOption by remember { mutableStateOf<String?>(null) }

    val sheetState = rememberModalBottomSheetState()
    var isSheetVisible by remember { mutableStateOf(sheetState.isVisible) }
    val scope = rememberCoroutineScope()

    Box(modifier = Modifier.fillMaxSize()) {
        AR(selectedOption)

        MenuButton(
            onClick = {
                scope.launch {
                    isSheetVisible = true
                    sheetState.expand()
                }
            },
            modifier = Modifier
                .align(Alignment.BottomEnd)
                .padding(bottom = 32.dp),
        )
    }

    if (isSheetVisible) {
        BottomSheet(
            state = sheetState,
            onOptionSelected = {
                selectedOption = it
                scope.launch { sheetState.hide() }
                    .invokeOnCompletion { isSheetVisible = false }
            },
            onDismissRequest = { isSheetVisible = false }
        )
    }
}

@Composable
private fun AR(selectedOption: String?) {
    ARScene(
        modifier = Modifier.fillMaxSize(),
        onGestureListener = rememberOnGestureListener(
            onSingleTapConfirmed = { event, node ->
                Log.d("AR", "Single tap confirmed (option: $selectedOption)")
            }
        )
    )
}

@Composable
private fun MenuButton(onClick: () -> Unit, modifier: Modifier = Modifier) {
    IconButton(onClick = onClick, modifier = modifier) {
        Icon(imageVector = Icons.Default.Menu, contentDescription = null)
    }
}

@Composable
private fun BottomSheet(
    state: SheetState,
    onOptionSelected: (String) -> Unit,
    onDismissRequest: () -> Unit,
) {
    ModalBottomSheet(
        onDismissRequest = onDismissRequest,
        sheetState = state,
        modifier = Modifier.navigationBarsPadding(),
    ) {
        LazyColumn(modifier = Modifier.padding(all = 8.dp)) {
            items(options) { option ->
                Text(text = option, modifier = Modifier.clickable { onOptionSelected(option) })
            }
        }
    }
}
WildOrangutan commented 4 days ago

I guess code should look like below. But I think this should be handled by rememberOnGestureListener. What do you think?

@Composable
private fun AR(selectedOption: String?) {
    val currentOption by rememberUpdatedState(selectedOption)
    ARScene(
        onGestureListener = rememberOnGestureListener(
            onSingleTapConfirmed = { event, node ->
                Log.d("AR", "Single tap confirmed (option: $currentOption)")
            }
        )
    )
}
ThomasGorisse commented 4 days ago

Hi,

Yes, you're right, a lambda shouldn't be remembered. Could you please create a PR to fix it?

Thanks

WildOrangutan commented 3 days ago

Could you please create a PR to fix it?

Can't make any promises, sorry. I'm not invested in your library yet. I'm still exploring my options for AR implementation.