Skip to content

Show realistic light and shadows sample #329

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

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

Conversation

darryl-lynch
Copy link
Contributor

@darryl-lynch darryl-lynch commented Mar 5, 2025

Description

Add realistic light and shadows sample

Links and Data

Issue: https://devtopia.esri.com/runtime/kotlin/issues/5316
vTest: https://runtime-kotlin.esri.com/view/all/job/vtest/job/sampleviewer/86/

What To Review

  • Review the code to make sure it is easy to follow like other samples on Android
  • README.md and README.metadata.json files
  • First time using SceneView, so please ensure usage is correct
  • Screenshot is not done yet as UI may change

How to Test

Run the sample on the sample viewer or the repo.

@colinanderson colinanderson self-requested a review March 7, 2025 10:55
Copy link
Collaborator

@colinanderson colinanderson left a comment

Choose a reason for hiding this comment

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

@darryl-lynch a few comments to look over

val hours = (sunTime / (60 * 60)) % 24
val minutes = (sunTime / 60) % 60
val seconds = sunTime % 60
lightingOptionsState.sunTime.value = OffsetDateTime.now(ZoneId.of("UTC"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think using now() here creates a bug because you are applying the slider time seconds to the current time.

I noticed this because running the sample I see shadows when the slider is all the way to PM when I'd assume night time and also shadows when the slider is all the way to AM again assuming it would be night.

Copy link
Contributor Author

@darryl-lynch darryl-lynch Mar 7, 2025

Choose a reason for hiding this comment

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

I think this is a timezone issue, since the scene jumps to Portland in PST but now() is being calculated relative to UTC. So the time on the slider and the time of the sun will be 8 hours off. I've updated the logic to use PST and set a default time of 15:00.

Copy link
Contributor Author

@darryl-lynch darryl-lynch Mar 7, 2025

Choose a reason for hiding this comment

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

Actually I feel like the way I've set the default time is probably breaking some kind of compose rule

edit: I think I've fixed that as well now

text = "AM",
modifier = Modifier.padding(12.dp)
)
Slider(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be good to give the slider tick marks every four hours like the design.
image

Ideally we'd make it look more like the design but looking at the slider doc it looks like the Compose slider isn't that customisable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked into it and it seems like it might be possible but it would require creating a whole new component for it. Swift doesn't have the labels so there is at least precedent for not having the ticks.

Copy link
Collaborator

@colinanderson colinanderson left a comment

Choose a reason for hiding this comment

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

@darryl-lynch I think you can simplify things if you take a look at all the https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/time/OffsetDateTime.html and related methods.

Copy link
Collaborator

@colinanderson colinanderson left a comment

Choose a reason for hiding this comment

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

@darryl-lynch I think ideally the model would just know the current time and the slider would call setSunTime which would update the model's time. You are sort of doing that but in an awkward way of recalculating from a midnight start time. If 3pm is the start time we want to use then the calculations should flow from that and the slider range could reflect that (see comment on -ve to +ve range).

I was expecting that setSunTime would just be something like

currentTime = currentTime.plusSeconds(seconds)

but that seems to introduce flickering.

)
val lightingOptionsState = LightingOptionsState(
mutableStateOf(
LocalDateTime.parse("2000-09-22T00:00:00").atZone(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This local date time appears twice in the code (here and line 95) but would be better if it was just a constant or some other fix.

You should be able to do something like

    val initialDateTime = LocalDateTime.parse("2000-09-22T00:00:00").atZone(ZoneId.of("US/Pacific"))

    /**
     * Update the sun time of the lighting options state used by the scene view.
     * Uses the range 0 seconds (12 am) to 86,340 seconds (11:59 pm).
     */
    fun setSunTime(sunTime: Int) {
        lightingOptionsState.sunTime.value = initialDateTime.plusSeconds(sunTime.toLong()).toInstant()
    }

},
// the range is 0 to 86,340 seconds ((60 seconds * 60 minutes * 24 hours) - 60 seconds),
// which means 12 am to 11:59 pm.
valueRange = 0f..86340f,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could the range not be from -ve to +ve and then you can just do plusSeconds against a known 3pm time?

@darryl-lynch darryl-lynch force-pushed the show_realistic_light_and_shadows branch from f54d51c to af4ca1b Compare March 26, 2025 15:43
Copy link
Collaborator

@colinanderson colinanderson left a comment

Choose a reason for hiding this comment

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

@darryl-lynch looks like what we discussed earlier 👍

@TADraeseke TADraeseke self-requested a review March 31, 2025 17:53
@TADraeseke
Copy link
Collaborator

I'm happy to take 2nd review on this one @darryl-lynch

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.

3 participants