Skip to content

chore: Multi-scene world - Realm dependant skybox time#7244

Open
dalkia wants to merge 5 commits intodevfrom
fix/multi-scene-world-skybox
Open

chore: Multi-scene world - Realm dependant skybox time#7244
dalkia wants to merge 5 commits intodevfrom
fix/multi-scene-world-skybox

Conversation

@dalkia
Copy link
Collaborator

@dalkia dalkia commented Feb 25, 2026

Pull Request Description

What does this PR change?

Support a fixed time of day defined at the realm level via the server about response. When the about JSON includes configurations.skybox.fixedHour, that value is applied as the world’s skybox time, with lower priority than Scene Component and Scene Metadata so scene-level control is preserved.

Test Instructions

Test Steps

  1. Realm time
  • Configure a world so the about response includes configurations.skybox.fixedHour (e.g. a valid hour in seconds).
  • Enter the world and confirm the skybox shows that time.
  1. Scene Component wins
  • In a scene, set time via a scene component.
  • Enter the world and confirm the scene component’s time is used, not the realm’s.
  1. Scene Metadata wins
  • Set time via scene metadata.
  • Enter the world and confirm the scene metadata time is used, not the realm’s.
  1. No realm time
  • Use a realm/about that does not set skybox.fixedHour (or uses an invalid value).
  • Confirm behavior is unchanged (e.g. global time or other states apply as before).

Code Review Reference

Please review our Code Review Standards before submitting.

@dalkia dalkia requested review from a team as code owners February 25, 2026 12:34
@github-actions github-actions bot requested a review from DafGreco February 25, 2026 12:34
@github-actions
Copy link
Contributor

github-actions bot commented Feb 25, 2026

badge

Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below.

Name Link
Commit a36e01b
Logs https://github.com/decentraland/unity-explorer/actions/runs/22413394498
Download Windows https://github.com/decentraland/unity-explorer/suites/58576434063/artifacts/5661242047
Download Windows S3 https://explorer-artifacts.decentraland.org/@dcl/unity-explorer/branch/fix/multi-scene-world-skybox/pr-19134-a36e01b/Decentraland_windows64.zip
Download Mac https://github.com/decentraland/unity-explorer/suites/58576434063/artifacts/5661134249
Download Mac S3 https://explorer-artifacts.decentraland.org/@dcl/unity-explorer/branch/fix/multi-scene-world-skybox/pr-19134-a36e01b/Decentraland_macos.zip
Built on 2026-02-25T20:38:50Z

Artifact size check

Target Size (MB / MiB) Cap (MB) Δ MB vs release Δ % vs release Result
windows64 317.8 / 303.0 320 0 -0.06% ✅ OK
macos 405.6 / 386.8 410 -2 -0.59% ✅ OK

Comparing against: v0.128.0-alpha

@dalkia dalkia changed the title chore: Multi-scene world realm dependant time chore: Multi-scene world - Realm dependant skybox time Feb 25, 2026
Copy link
Collaborator

@lorux0 lorux0 left a comment

Choose a reason for hiding this comment

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

Clean! 👌

Copy link
Contributor

@Ludmilafantaniella Ludmilafantaniella left a comment

Choose a reason for hiding this comment

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

Verified on Windows and Mac following the provided test scenarios.

QA validation performed using configured scene https://sdk-team-cdn.decentraland.org/ipfs/sdk7-test-scenes-skybox-time-test-scenes-next
:

  • Realm-level skybox.fixedHour is correctly applied when present
  • Scene Component time properly overrides realm configuration
  • Scene Metadata time correctly takes highest priority
  • Behavior remains unchanged when fixedHour is missing or invalid
  • No regressions observed during testing

The priority chain works as intended and the implementation behaves as expected.

Status: Approved for merge 🚀

7244-evi.mp4

✅Smoke test performed:

  • ✔️ Skybox functionality in worlds and Genesis City
  • ✔️ Backpack and wearables in world
  • ✔️ Emotes in world and in backpack
  • ✔️ Teleport with map/coordinates/Jump In
  • ✔️ Chat and multiplayer
  • ✔️ Camera
  • ✔️ Skybox

@dalkia
Copy link
Collaborator Author

dalkia commented Feb 25, 2026

@claude review

@claude
Copy link

claude bot commented Feb 25, 2026

Claude finished @dalkia's task in 3m 14s —— View job


Code Review

  • Gather PR context
  • Read changed files
  • Analyze code quality and potential issues
  • Post review findings

Issues Found

1. Unit Mismatch — Hours vs Seconds (Potential Bug)

Location: Explorer/Assets/DCL/SkyBox/RealmSkyboxState.cs:63
Also affects: Explorer/Assets/DCL/NetworkDefinitions/ServerConfiguration.cs:9, Explorer/Assets/DCL/NetworkDefinitions/IRealmData.cs:47

Problem: SkyboxSettingsAsset.NormalizeTime(float time) divides by SECONDS_IN_DAY = 86400, meaning it expects a value in seconds. However, the server field is named fixedHour (implying hours), and the IRealmData interface property is named SkyboxFixedHour. If the server truly sends a value in hours (e.g., 12 for noon), then:

NormalizeTime(12) = 12 / 86400 ≈ 0.000139  →  nearly midnight, not noon

The correct conversion for an hour-based value would be:

NormalizeTime(12 * 3600) = 43200 / 86400 = 0.5  →  noon ✓

The PR description test instructions read "a valid hour in seconds", which is self-contradictory. This same concern was raised by reviewer lorux0 and remains unanswered.

The existing feature-flag path in SkyboxPlugin.cs uses an explicitly named FixedTimeInSeconds (uint) to avoid this ambiguity — the new realm path lacks that clarity.

Fix (if the server sends hours): Apply the conversion before calling NormalizeTime:

// RealmSkyboxState.cs:63
float normalizedTime = SkyboxSettingsAsset.NormalizeTime(realmData.SkyboxFixedHour.Value * 3600f);

Fix (if the server sends seconds but the naming is wrong): Rename the field throughout the chain:

  • ServerSkyboxConfig.fixedHourfixedTimeInSeconds
  • IRealmData.SkyboxFixedHourSkyboxFixedTimeInSeconds
  • RealmData.SkyboxFixedHourSkyboxFixedTimeInSeconds

Why it matters: The skybox would show an almost-midnight sky regardless of the configured world time. QA results suggest it is currently passing, meaning either the server sends seconds (naming is wrong) or the test world happened to use a value that appeared plausibly correct. Either way, the ambiguity must be resolved.

Fix this →


2. Missing TransitionMode in RealmSkyboxState.Enter()

Location: Explorer/Assets/DCL/SkyBox/RealmSkyboxState.cs:34-41

Problem: Every other state that locks the skybox time explicitly configures settings.TransitionMode before delegating to transition. SceneMetadataState.Enter() calls TryApplySceneMetadata() which sets settings.TransitionMode = GetTransitionMode(metadata). RealmSkyboxState.Enter() skips this — the transition mode is whatever was left by the previous state, which could be BACKWARD if the user was in a scene that used backward transitions.

public void Enter()
{
    sceneRestrictionController.PushSceneRestriction(...);
    settings.IsDayCycleEnabled = false;
    // TransitionMode is not set — stale value from previous state is used
    ApplyFixedHour();
    transition.Enter();
}

Fix:

public void Enter()
{
    sceneRestrictionController.PushSceneRestriction(...);
    settings.IsDayCycleEnabled = false;
    settings.TransitionMode = TransitionMode.FORWARD;
    ApplyFixedHour();
    transition.Enter();
}

Summary

Two issues need resolution before merge:

  1. Unit ambiguity (hours vs seconds) in the fixedHourNormalizeTime pipeline — must be confirmed and either corrected in code or corrected in naming.
  2. Missing TransitionMode reset in RealmSkyboxState.Enter() — can cause unexpected backward-direction transitions.

REVIEW_RESULT: FAIL ❌

dalkia and others added 3 commits February 25, 2026 16:28
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
Signed-off-by: Juan Ignacio Molteni <juanignaciomolteni@gmail.com>
@dalkia dalkia enabled auto-merge (squash) February 25, 2026 19:54
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