Skip to content

Skybox time settings - Versioning SceneMetada component - #1184

Merged
nicoecheza merged 24 commits into
mainfrom
feat/skybox-time-settings
Aug 6, 2025
Merged

Skybox time settings - Versioning SceneMetada component#1184
nicoecheza merged 24 commits into
mainfrom
feat/skybox-time-settings

Conversation

@alejandralevy

@alejandralevy alejandralevy commented Jul 31, 2025

Copy link
Copy Markdown
Contributor
  • Add skybox config on scene settings (skybox time and trasition)
  • The skybox sets the time of the scene while the transition sets how the skybox moves from the current time to the scene time
  • When the skybox value is setted, it cannot be changed from the scene itself
  • Run migration to move data from SceneMetadata componente to SceneMetadata-v1 component
  • Support easy versioning for future changes of properties

To test:
As it includes a migration of the scene metada componente, please test this feature in new scenes and old scenes.

Screen.Recording.2025-08-04.at.16.25.44.mov
Screen.Recording.2025-07-31.at.13.41.57.mov

@github-actions

github-actions Bot commented Jul 31, 2025

Copy link
Copy Markdown
Contributor

Test this pull request

  • The @dcl/sdk package can be tested in scenes by running

    npm install "https://sdk-team-cdn.decentraland.org/@dcl/js-sdk-toolchain/branch/feat/skybox-time-settings/dcl-sdk-7.9.3-16727417707.commit-d248c5e.tgz"
  • To test with npx init

    export SDK_COMMANDS="https://sdk-team-cdn.decentraland.org/@dcl/js-sdk-toolchain/branch/feat/skybox-time-settings/dcl-sdk-commands-7.9.3-16727417707.commit-d248c5e.tgz"
    npx $SDK_COMMANDS init
  • The @dcl/inspector package can be tested by visiting this url

    • Or by installing it via NPM
    npm install "https://sdk-team-cdn.decentraland.org/@dcl/js-sdk-toolchain/branch/feat/skybox-time-settings/@dcl/inspector/dcl-inspector-7.9.3-16727417707.commit-d248c5e.tgz"
  • The /changerealm command to test test in-world

    /changerealm https://sdk-team-cdn.decentraland.org/ipfs/feat/skybox-time-settings-e2e
    
  • You can preview this build entering:
    https://playground.decentraland.org/?sdk-branch=feat/skybox-time-settings

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jul 31, 2025

Copy link
Copy Markdown

Deploying js-sdk-toolchain with  Cloudflare Pages  Cloudflare Pages

Latest commit: 5f40711
Status: ✅  Deploy successful!
Preview URL: https://cd6f4b3b.js-sdk-toolchain.pages.dev
Branch Preview URL: https://feat-skybox-time-settings.js-sdk-toolchain.pages.dev

View logs

Comment thread packages/@dcl/inspector/src/components/EntityInspector/SceneInspector/types.ts Outdated
onChange={handleSkyboxAutoChange}
/>
<RangeHourField
value={componentValue.skyboxConfig?.fixedTime || MIDDAY_SECONDS}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

in case fixedTime is 0, this will default to MIDDAY_SECONDS. Is this correct? (maybe replace it with ??)

skyboxConfig: {
fixedTime: Number(inputs.skyboxConfig.fixedTime || MIDDAY_SECONDS),
transitionMode: TransitionMode.TM_FORWARD
},

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

same as above for all the previous ones

rightLabel?: string
error?: string | boolean
info?: React.ReactNode
isValidValue?: (value: any) => boolean

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

value: number

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I finally removed this prop

export type Props = React.InputHTMLAttributes<Omit<HTMLElement, 'type'>> & {
label?: React.ReactNode
rightLabel?: string
error?: string | boolean

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this one should be string | undefined

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Also removed the error prop

info,
value = 0,
min = 0,
max = 86400,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

maybe give this a meaningful name? 🤔

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

the min and the max value are now set in the component itself

Comment on lines +41 to +47
useEffect(() => {
const numValue = Number(value)
if (numValue !== inputValue) {
setInputValue(numValue)
setTextValue(formatHour(numValue))
}
}, [value])

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this one is not necessary 🤔

@alejandralevy alejandralevy Aug 4, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it updates the state after a value is received by prop, because the value is not being directly used, it's on the useState

Comment on lines +38 to +39
const [inputValue, setInputValue] = useState<number>(Number(value))
const [textValue, setTextValue] = useState(formatHour(Number(value)))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

seems like this two always change together, maybe we can const [value, setValue] = useState<{ timeInSeconds: number; timeInHHMM: string; }> ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

merged

Comment on lines +104 to +120
if (value.length <= 5) {
setTextValue(value)

// Si es un formato válido de hora, actualizar el slider
if (isValidTimeFormat(value)) {
const seconds = parseTimeInput(value)
if (seconds >= Number(min) && seconds <= Number(max)) {
setInputValue(seconds)
onChange &&
onChange({
...e,
target: { ...e.target, value: seconds.toString() }
} as React.ChangeEvent<HTMLInputElement>)
}
}
}
},

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we should split the logic for handling changes on the value input and the text input so we end up with something like:

  • handleChangeRange => does what handleChange is currently doing + call handleChange
  • handleChangeTime => does what handleChangeTextField is currently doing + call handleChange
  • handleChange => check if whole value ({ timeInSeconds, timeInHHMM }) is valid and calls props.onChange

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll double check this logic because now that I'm using a native time input, the validation is done by the input itself, so I don't think I need most of these validations

Comment on lines +38 to +56
export function migrateSceneMetadata(engine: IEngine) {
const latestComponent = getCompositeLatestSceneComponent(engine)

if (!latestComponent) return

const { component, value } = latestComponent

const latestComponentVersion = getLatestSceneComponentVersion()
const isRunningLatestVersion = component.componentName === latestComponentVersion.key

if (isRunningLatestVersion) return

const oldComponent = component
oldComponent.deleteFrom(engine.RootEntity)
engine.removeComponentDefinition(oldComponent.componentName)

const SceneMetadata = engine.getComponent(latestComponentVersion.key) as EditorComponents['Scene']
SceneMetadata.createOrReplace(engine.RootEntity, value)
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🚀 🚀 🚀 🚀

(remember to delete all definitions except the latest one)

@codecov

codecov Bot commented Aug 4, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.61290% with 26 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.53%. Comparing base (63d0108) to head (06af810).
⚠️ Report is 24 commits behind head on main.

Files with missing lines Patch % Lines
...er/host/utils/migrations/migrate-scene-metadata.ts 86.66% 7 Missing and 1 partial ⚠️
...nspector/src/components/ui/RangeHourField/types.ts 0.00% 6 Missing and 1 partial ⚠️
.../inspector/src/lib/sdk/components/SceneMetadata.ts 95.56% 7 Missing ⚠️
...components/EntityInspector/SceneInspector/types.ts 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1184      +/-   ##
==========================================
- Coverage   62.00%   59.53%   -2.47%     
==========================================
  Files         621      631      +10     
  Lines       20903    23065    +2162     
  Branches     2536     2585      +49     
==========================================
+ Hits        12960    13731     +771     
- Misses       7501     8889    +1388     
- Partials      442      445       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nicoecheza
nicoecheza merged commit d7bbd0b into main Aug 6, 2025
9 checks passed
@nicoecheza
nicoecheza deleted the feat/skybox-time-settings branch August 6, 2025 13:22
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