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

Add shadows_disabled macro in Compatibility renderer #84416

Merged
merged 1 commit into from
Dec 4, 2023
Merged

Add shadows_disabled macro in Compatibility renderer #84416

merged 1 commit into from
Dec 4, 2023

Conversation

jsjtxietian
Copy link
Contributor

Fixes #84386

@jsjtxietian jsjtxietian requested a review from a team as a code owner November 3, 2023 15:49
@YuriSizov YuriSizov added this to the 4.2 milestone Nov 3, 2023
@clayjohn clayjohn modified the milestones: 4.2, 4.3 Nov 7, 2023
@clayjohn clayjohn added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Nov 7, 2023
clayjohn
clayjohn approved these changes Nov 7, 2023
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.

Seems fine to me.

Let's merge this for 4.3 as we are in the final stages of 4.2. We can cherrypick this for 4.2.1

AThousandShips
AThousandShips reviewed Nov 7, 2023
View reviewed changes
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Some accidental changes that should be restored

drivers/gles3/shaders/scene.glsl Outdated Show resolved Hide resolved
drivers/gles3/shaders/scene.glsl Outdated Show resolved Hide resolved
Calinou
Calinou requested changes Nov 7, 2023
View reviewed changes
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally. This PR breaks rendering in an empty project with a few meshes, even if they have no material assigned: test_pr_84416.zip

ERROR: SceneShaderGLES3: Fragment shader compilation failed:
0(1540) : error C7102: unmatched #endif

   at: _display_error_with_code (drivers/gles3/shader_gles3.cpp:252)

@jsjtxietian
Copy link
Contributor Author

jsjtxietian commented Nov 7, 2023 •
edited
Loading

Hi @Calinou Did you use the last commit, I do miss a macro there when try to fix the clang format issue.

This one is broken and the latest one is fine:

image

Calinou
Calinou approved these changes Nov 7, 2023
View reviewed changes
Copy link
Member

@Calinou Calinou left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Tested latest revision, it works as expected. Enabling Disable Receive Shadows on the material correctly disables shadow receiving for that material for all light types.

@jsjtxietian
Copy link
Contributor Author

Thanks! I will try not to update my llvm too aggressively next time :(

@akien-mga akien-mga changed the title Add shadows_disabled macro in Compatibility renderer Add shadows_disabled macro in Compatibility renderer Dec 4, 2023
@akien-mga akien-mga merged commit 6f16e3f into godotengine:master Dec 4, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@YuriSizov
Copy link
Contributor

Cherry-picked for 4.2.1.

@YuriSizov YuriSizov removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Dec 5, 2023
@jsjtxietian jsjtxietian deleted the shadow_disabled branch December 9, 2023 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AThousandShips AThousandShips AThousandShips left review comments

@clayjohn clayjohn clayjohn approved these changes

@Calinou Calinou Calinou approved these changes

Assignees
No one assigned
Labels
bug topic:rendering topic:3d
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.

Shadows cannot be disabled in the material in the Compatibility renderer
6 participants
@jsjtxietian @akien-mga @YuriSizov @Calinou @clayjohn @AThousandShips