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

fix: [Graphics] Fix light component API #2215

Merged
merged 1 commit into from
May 29, 2024
Merged

Conversation

Eideren
Copy link
Collaborator

@Eideren Eideren commented Apr 5, 2024

PR Details

LightComponent.Light.Shadow is an abstract LightShadowMap shared across all light types, the light rendering logic expects specific combination of shadow types per light types. So, users could set LightPointShadowMapRendererCubeMap on a Directional Light for example, leading to unexpected rendering issue or exceptions.
This change ensures that each light's shadow can only be set to a supported shadow map type.

Motivation and Context

Fixes poor usage of inheritance and composition principles. There's more work on the XShadowMapRenderer side, but while that logic isn't that well written, it won't crash on us.

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix
  • New feature
  • Breaking change - users will no longer be able to set unexpected shadowmap types. Or reference DirectLightBase as that type has been removed, I could resurrect it as long as it doesn't implement IDirectLight, but it wouldn't be very useful then.

Checklist

  • My change requires a change to the documentation.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have built and run the editor to try this change out.

@Doprez
Copy link
Contributor

Doprez commented Apr 5, 2024

I did a quick test to see if I could recreate the issue and it seems better right now.
Directional light has the proper Shadow map assigned
image

And spotlight also seems correct
image

Only thing I didnt test was if I were to add a new shadowmap if the dropdown would appear again. But I dont have a secondary I could test with, I guess I could just make a duplicated shadow map of one of these and see.

Edit: I didnt realise the change would assign the ShadowMap directly. So you would have to create a new Light by inheriting ColorLightBase and IDirectLight. This makes sense to me but maybe @tebjan should speak on this for the rendering side?

@Eideren Eideren changed the title [Graphics] Fix light component API fix: [Graphics] Fix light component API May 29, 2024
@Eideren Eideren merged commit 817da70 into stride3d:master May 29, 2024
13 checks passed
@Eideren
Copy link
Collaborator Author

Eideren commented May 29, 2024

Thanks a bunch for reviewing @Kryptos-FR :)

@Eideren Eideren deleted the light_fix branch May 29, 2024 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants