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

Animate images with image overlay #293

Open
wants to merge 28 commits into
base: v.next
Choose a base branch
from

Conversation

TADraeseke
Copy link
Collaborator

@TADraeseke TADraeseke commented Dec 17, 2024

Description

PR to add a new Kotlin sample "Animate images with image overlay" in Visualization category.

Links and Data

Sample issue: https://devtopia.esri.com/runtime/kotlin/issues/1138

What To Review

@shubham7109 shubham7109 added the New sample New Kotlin sample using ArcGIS Maps SDK label Dec 19, 2024
@shubham7109 shubham7109 self-requested a review December 19, 2024 22:47
TADraeseke and others added 15 commits December 20, 2024 17:35
…ri/arcgismaps/sample/animateimageswithimageoverlay/DownloadActivity.kt

Co-authored-by: Shubham Sharma <[email protected]>
…ri/arcgismaps/sample/animateimageswithimageoverlay/components/AnimateImagesWithImageOverlayViewModel.kt

Co-authored-by: Shubham Sharma <[email protected]>
…ri/arcgismaps/sample/animateimageswithimageoverlay/screens/AnimateImagesWithImageOverlayScreen.kt

Co-authored-by: Shubham Sharma <[email protected]>
…ri/arcgismaps/sample/animateimageswithimageoverlay/screens/AnimateImagesWithImageOverlayScreen.kt

Co-authored-by: Shubham Sharma <[email protected]>
…ri/arcgismaps/sample/animateimageswithimageoverlay/screens/AnimateImagesWithImageOverlayScreen.kt

Co-authored-by: Shubham Sharma <[email protected]>
…ri/arcgismaps/sample/animateimageswithimageoverlay/components/AnimateImagesWithImageOverlayViewModel.kt

Co-authored-by: Shubham Sharma <[email protected]>
…ri/arcgismaps/sample/animateimageswithimageoverlay/components/AnimateImagesWithImageOverlayViewModel.kt

Co-authored-by: Shubham Sharma <[email protected]>
…ri/arcgismaps/sample/animateimageswithimageoverlay/components/AnimateImagesWithImageOverlayViewModel.kt

Co-authored-by: Shubham Sharma <[email protected]>
…ri/arcgismaps/sample/animateimageswithimageoverlay/components/AnimateImagesWithImageOverlayViewModel.kt

Co-authored-by: Shubham Sharma <[email protected]>
…ri/arcgismaps/sample/animateimageswithimageoverlay/components/AnimateImagesWithImageOverlayViewModel.kt

Co-authored-by: Shubham Sharma <[email protected]>
…ri/arcgismaps/sample/animateimageswithimageoverlay/components/AnimateImagesWithImageOverlayViewModel.kt

Co-authored-by: Shubham Sharma <[email protected]>
…ri/arcgismaps/sample/animateimageswithimageoverlay/components/AnimateImagesWithImageOverlayViewModel.kt

Co-authored-by: Shubham Sharma <[email protected]>
…ri/arcgismaps/sample/animateimageswithimageoverlay/components/AnimateImagesWithImageOverlayViewModel.kt

Co-authored-by: Shubham Sharma <[email protected]>
@TADraeseke
Copy link
Collaborator Author

Thanks for all the detailed suggestions @shubham7109 -- changes made!

Copy link
Collaborator

@shubham7109 shubham7109 left a comment

Choose a reason for hiding this comment

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

@TADraeseke The sample looks great. Thanks for making the changes, just a few minor enhancement suggestions.

Comment on lines +157 to +166
val period = when (fps) {
60 -> 17 // 1000ms/17 = 60 fps
30 -> 33 // 1000ms/33 = 30 fps
15 -> 67 // 1000ms/67 = 15 fps
else -> 0
}
timer?.cancel()
timer = fixedRateTimer("Image overlay timer", period = period.toLong()) {
addNextImageFrameToImageOverlay()
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

fps could be part of an internal flow that creates a new fixedRateTimer on each update. This would allow us to change the fps of the animation in real-time.

}
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could use a @Preview function here:

Suggested change
}
}
@Preview(showBackground = true)
@Preview(uiMode = Configuration.UI_MODE_NIGHT_YES, showBackground = true)
@Composable
fun ClusterInfoContentPreview() {
SampleAppTheme {
Surface {
ImageOverlayMenu(
isStarted = true,
fps = 60,
opacity = 1F,
fpsOptions = listOf(60, 30, 15),
onFpsOptionSelected = { },
onOpacityChanged = { },
onIsStartedChanged = {}
)
}
}
}

Comment on lines +138 to +140
updateFpsOption: KFunction1<Int, Unit>,
updateOpacity: KFunction1<Float, Unit>,
updateIsStarted: KFunction1<Boolean, Unit>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Name change:

Suggested change
updateFpsOption: KFunction1<Int, Unit>,
updateOpacity: KFunction1<Float, Unit>,
updateIsStarted: KFunction1<Boolean, Unit>
onFpsOptionSelected: (Int) -> Unit,
onOpacityChanged: (Float) -> Unit,
onIsStartedChanged: (Boolean) -> Unit

updateIsStarted: KFunction1<Boolean, Unit>
) {

var selectedFps by remember { mutableIntStateOf(60) }
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be removed in favor of the fps provided by the view model.

Suggested change
var selectedFps by remember { mutableIntStateOf(60) }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New sample New Kotlin sample using ArcGIS Maps SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants