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.

Sign up for GitHub

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

Jump to bottom

Fix LightOccluder2D SDF Collision Enable/Disable #90883

Merged
merged 1 commit into from Apr 23, 2024
Merged

Fix LightOccluder2D SDF Collision Enable/Disable #90883

merged 1 commit into from Apr 23, 2024

Conversation

cosparks
Copy link
Contributor

SDF collision enabled parameter wasn't being assigned to anything so disabling SDF collisions on LightOccluder2D had no effect.

With this change, unchecking SDF Collisions will actually disable collisions between gpu particles and light occluders.

closes #90882

@cosparks cosparks requested a review from a team as a code owner April 19, 2024 03:16
@cosparks cosparks changed the title Fixes SDF Collision Enable/Disable Fix LightOccluder2D SDF Collision Enable/Disable Apr 19, 2024
@clayjohn clayjohn added this to the 4.3 milestone Apr 19, 2024
@Calinou Calinou added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Apr 19, 2024
@cosparks
Copy link
Contributor Author

cosparks commented Apr 21, 2024 •
edited

There should be a hold on this as setting LightOccluderInstance::enabled to false also disables shadow casting, which I would imagine isn't the desired behaviour.

On the other hand, setting LightOccludeInstance::sdf_collision to false has no effect, so the issue probably lies somewhere deeper than in RendererCanvasCull::canvas_light_occluder_set_as_sdf_collision

I've updated the PR

@cosparks
Copy link
Contributor Author

cosparks commented Apr 21, 2024 •
edited

@reduz I saw that you implemented sdf for 2d shaders.

Was the original idea that LightOccluder2Ds with sdf_collision set to false wouldn't be drawn to the sdf texture? If you could take a look at this that would be awesome.

Edit: I just looked at the documentation for LightOccluder2D and it seems that this is the case, so this is probably the right fix.

clayjohn
clayjohn approved these changes Apr 22, 2024
View reviewed changes
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Looks great! The final step before merging is to squash all the commits together so that the whole PR only contains 1 big commit with all your changes. We like to merge one commit at a time to keep the git history clean and navigable.

If you don't know how to do that, we have a helpful tutorial in the official documentation https://docs.godotengine.org/en/latest/community/contributing/pr_workflow.html#the-interactive-rebase

@cosparks
Fixes SDF Collision Enable/Disable
6d0dca7 
- sets LightOccluderInstance field when sdf collision is updated
- adds check for light occluder sdf_collision field in 2d renderers
@cosparks
Copy link
Contributor Author

Cool! I just squashed my commits. Thanks for linking the docs.

@akien-mga akien-mga merged commit 99cff79 into godotengine:master Apr 23, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@cosparks cosparks deleted the fix-sdf-collision-2d branch April 28, 2024 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clayjohn clayjohn clayjohn approved these changes

Assignees
No one assigned
Labels
bug cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release topic:rendering topic:2d
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.

LightOccluder2D -- SDF Collision is always enabled
4 participants
@cosparks @akien-mga @clayjohn @Calinou