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

Consider visual layers for DirectionalLight #95379

Merged
merged 1 commit into from
Aug 16, 2024

Conversation

SlashScreen
Copy link
Contributor

@SlashScreen SlashScreen commented Aug 11, 2024

Here, I have applied the patch provided by @majikayogames, in an updated line number to reflect the updated rendering engine. This should close #74545.
A concern was brought up about the directional lights not being considered for culling earlier in the function, but I don't believe there is any way to be confident you can cull a directional light without first rendering the shadowmap. If I am wrong, let me know.
I have been running a version of Godot that contains this patch, and I have seen no issues thus far.

@SlashScreen SlashScreen requested a review from a team as a code owner August 11, 2024 02:26
@Calinou Calinou added bug topic:rendering topic:3d cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Aug 11, 2024
@Calinou Calinou added this to the 4.4 milestone Aug 11, 2024
@AThousandShips AThousandShips changed the title Apply patch Consider visual layers for DirectionalLight Aug 11, 2024
@akien-mga
Copy link
Member

Could you amend the commit message to be more explicit about what said patch does? E.g. like @AThousandShips used to edit the PR title. See https://github.com/godotengine/godot/blob/master/CONTRIBUTING.md#format-your-commit-messages-with-readability-in-mind

@SlashScreen SlashScreen force-pushed the fix_dir_light_layers branch from 80da075 to 364dd69 Compare August 12, 2024 18:31
@SlashScreen
Copy link
Contributor Author

Could you amend the commit message to be more explicit about what said patch does? E.g. like @AThousandShips used to edit the PR title. See https://github.com/godotengine/godot/blob/master/CONTRIBUTING.md#format-your-commit-messages-with-readability-in-mind

Done. Although, I seem to have accidentally included the last PR in my PR. I'm not entirely sure how to fix that.

@AThousandShips
Copy link
Member

AThousandShips commented Aug 12, 2024

You need to drop it with git rebase -i master, and just replace keep pick with drop for the first one

@SlashScreen
Copy link
Contributor Author

You need to drop it with git rebase -i master, and just replace keep with drop for the first one

Only my commit shows up in git rebase -i master, and did you mean pick, not keep?

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, it works as expected in all rendering methods.

@AThousandShips
Copy link
Member

When you finish the rebase, is the broken commit still there? It should probably have been replaced with the proper one, check the log with git log

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.

The change to renderer_scene_cull looks great!

You will need to remove your accidental commit before this is ready to merge though

@SlashScreen SlashScreen force-pushed the fix_dir_light_layers branch from 364dd69 to ac5e253 Compare August 12, 2024 21:56
@SlashScreen
Copy link
Contributor Author

The change to renderer_scene_cull looks great!

You will need to remove your accidental commit before this is ready to merge though

Right-o. I think I removed it. It says "commits : 1" now.

@clayjohn
Copy link
Member

The change to renderer_scene_cull looks great!
You will need to remove your accidental commit before this is ready to merge though

Right-o. I think I removed it. It says "commits : 1" now.

It looks great now. Thank you!

@SlashScreen
Copy link
Contributor Author

Is this still slated for a maintenance release?

@clayjohn
Copy link
Member

Is this still slated for a maintenance release?

Yes, this should be fine to merge for 4.3.1

@SlashScreen
Copy link
Contributor Author

SlashScreen commented Aug 12, 2024 via email

@clayjohn
Copy link
Member

Alright, noted. I’m not asking to be pushy, but why not in 4.3? I don’t yet know the thought process for what gets included for a main release vs what is moved to a maintenance release. Is it simply that it’s an unproven fix?

Message ID: @.***>

We are just too late in the 4.3 process. In the Release Candidate phase we only merge urgent fixes and we have already done 3 RCs. We have to draw the line somewhere or else we will never release 4.3

@SlashScreen
Copy link
Contributor Author

SlashScreen commented Aug 12, 2024 via email

@SlashScreen
Copy link
Contributor Author

If majikayo isn't already a contributor, is there a way to include them as a contributor? they did most of the work here, hunting down the issue and providing a fix. All I did was put it in the right spot.

@AThousandShips
Copy link
Member

You can add them as a co-author, you need to find their GitHub handle from some commit and add:


Co-authored-by: [username] <[email]>

The empty line is necessary and the details can be gotten from a commit of theirs with git log

@SlashScreen
Copy link
Contributor Author

You can add them as a co-author, you need to find their GitHub handle from some commit and add:


Co-authored-by: [username] <[email]>

The empty line is necessary and the details can be gotten from a commit of theirs with git log

Add this to what? The commit message?

@AThousandShips
Copy link
Member

Yes exactly

Co-authored-by: majikayogames <152851004+majikayogames@users.noreply.github.com>
@SlashScreen SlashScreen force-pushed the fix_dir_light_layers branch from ac5e253 to 4457b11 Compare August 13, 2024 15:46
@SlashScreen
Copy link
Contributor Author

Got it. Thanks.

@SlashScreen
Copy link
Contributor Author

Alright, is it showtime?

@clayjohn
Copy link
Member

Alright, is it showtime?

It's showtime 😎

@akien-mga akien-mga merged commit 94e9b2e into godotengine:master Aug 16, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

Cherry-picked for 4.3.1.

@akien-mga akien-mga removed the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Sep 16, 2024
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.

Visual layers are ignored for DirectionalLight
5 participants