Skip to content

ADR 276: light sources#276

Draft
nearnshaw wants to merge 7 commits into
mainfrom
light-sources
Draft

ADR 276: light sources#276
nearnshaw wants to merge 7 commits into
mainfrom
light-sources

Conversation

@nearnshaw

Copy link
Copy Markdown
Member

No description provided.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Nov 13, 2024

Copy link
Copy Markdown

Deploying adr with  Cloudflare Pages  Cloudflare Pages

Latest commit: 5f4b636
Status: ✅  Deploy successful!
Preview URL: https://7e27a6c1.adr-cvq.pages.dev
Branch Preview URL: https://light-sources.adr-cvq.pages.dev

View logs

@aixaCode aixaCode changed the title light soruces light sources Nov 14, 2024
Comment thread content/ADR-253-light-sources.md Outdated
Comment thread content/ADR-253-light-sources.md Outdated
Comment thread content/ADR-253-light-sources.md Outdated
Comment thread content/ADR-253-light-sources.md Outdated
@aixaCode aixaCode changed the title light sources ADR-253: light sources Nov 14, 2024
@aixaCode aixaCode changed the title ADR-253: light sources ADR 276: ight sources Nov 14, 2024
@aixaCode aixaCode changed the title ADR 276: ight sources ADR 276: light sources Nov 14, 2024

@leanmendoza leanmendoza left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for putting this ADR for light sources! This is a crucial feature for Decentraland.
I want to take this place to debate this, given the previous work we've done in the protocol. Please take a look before reading the next part:

I had written a whole text to comment as a review, but then I split into different topics and comment each part of the ADR to keep the conversation cleaner :)

Comment thread content/ADR-253-light-sources.md Outdated
Comment thread content/ADR-253-light-sources.md
Comment thread content/ADR-253-light-sources.md Outdated
Comment thread content/ADR-253-light-sources.md
Comment thread content/ADR-253-light-sources.md
- The creator can chose the shadow resolution as a setting on each light source
- The shadow resolution is picked by the player in the user’s settings, as a global option. If they have low settings they’ll always use low res

## Limitations

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I suggest removing the "Limitations and Considerations" section from this ADR. Here's why:

  1. Separation of Concerns
  • This ADR should focus on defining the light component interfaces and their behavior
  • Implementation details like performance limits should be handled separately:
    • Engine-specific optimizations
    • Platform-specific limitations
    • Scene-level restrictions
  1. Flexibility for Implementations
    Let each explorer/renderer decide how to handle:
  • Maximum number of lights
  • Shadow map allocation
  • Performance optimizations
  • Mobile vs desktop capabilities
  • Low-end vs high-end configurations
  1. SDK's Role
    The SDK can provide:
  • Platform-specific warnings
  • Best practice guidelines
  • Performance recommendations
  • Scene validation tools
  1. Better Documentation Structure
    These topics deserve their own documentation:
  • Performance best practices ADR
  • Scene optimization guide
  • Platform compatibility matrix
  • Implementation guidelines

This approach:

  • Keeps the component specification clean and focused
  • Allows for platform-specific optimizations
  • Enables future improvements without spec changes
  • Lets implementations evolve independently

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I’m okay with removing the Limitations and Considerations section, but I’d like to challenge this approach slightly. These limitations guide how each client should behave, ensuring consistency across implementations. Without them, we risk significant divergence in how the feature works across clients, which might shift responsibility to creators to adapt to different behaviors.

Creating a separate ADR or RFC for these constraints could work, but we’d need to align on how the protocol should remain generic while still providing a baseline for consistent behavior. I also see the value in keeping this ADR abstract to allow for experimentation and flexibility, but it’s crucial we ensure creators aren’t left with unpredictable experiences due to discrepancies between clients.

Are we ok with having that differences between clients?
If we move this to another ADR, then we need to provide a clear guidelines, tools and documentation for creators to navigate that different clients.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I agree with both, the ADR should focus on the protocol, but sometimes the protocol is more than just the data sent via the transport, it's also about the expected behavior.

I simplified these lines to something more generic:

Each engine is free to determine considerations like shadow resolutions, or putting a limit on the number of shadows being computed and how to prioritize these. It's recommendable to make these variables dependent on user quality settings.

Signed-off-by: Alejandro Alvarez Melucci <163010988+AlejandroAlvarezMelucciDCL@users.noreply.github.com>
Fixed example code

Signed-off-by: Alejandro Alvarez Melucci <163010988+AlejandroAlvarezMelucciDCL@users.noreply.github.com>
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.

4 participants