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 ambient colour being black not rendering point lights. #24

Merged
merged 5 commits into from
Aug 19, 2024

Conversation

isidornygren
Copy link
Contributor

@isidornygren isidornygren commented Aug 17, 2024

Summary

Uses the ambient light colour as the base for point light rendering instead of multiplying it later with the point light colour. This renders without issues if the ambient colour is black.

Fixes #22

Copy link
Owner

@jgayfer jgayfer left a comment

Choose a reason for hiding this comment

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

Hi, thanks for the PR!

While I believe the proposed change is the correct way to approach this problem (from an objective standpoint), it does make lights much brighter than they were prior.

Before After
Screenshot 2024-08-17 at 2 29 13 PM Screenshot 2024-08-17 at 2 28 50 PM

This can be fixed in the dungeon example by knocking the light intensity from 25.0 to 2.0, which is a bit more of a sane value, from a "numbers" point of view.

I like the change, but am mostly unsure what to do from a release point of view, as this would be breaking. I suppose a 0.4 release is the answer for that.

Copy link
Owner

@jgayfer jgayfer left a comment

Choose a reason for hiding this comment

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

We should probably:

  • Update the intensity value on point lights in our examples to keep the same look (I believe this is just the dungeon example)
  • Add an entry to CHANGELOG.md under the Unreleased heading documenting this change, with a reference to this PR number (#24)

Then we can move ahead with the PR.
Thank you!

@isidornygren
Copy link
Contributor Author

Howdy, and thanks for the quick response!

We should probably:

  • Update the intensity value on point lights in our examples to keep the same look (I believe this is just the dungeon example)

  • Add an entry to CHANGELOG.md under the Unreleased heading documenting this change, with a reference to this PR number (#24)

Then we can move ahead with the PR. Thank you!

Absolutely. I've updated the branch with updated intensity values for the dungeon and the multiple examples as they seemed like the only ones affected. I opted for an intensity value of 1.0 in the multiple example due to it being a nicer value than 0.5 (which is the value that more closely renders similarly to prior the change), I think it still looks acceptable though:

Example Before After
Basic Screenshot 2024-08-18 at 10 04 07 Screenshot 2024-08-18 at 10 11 06
Dungeon Screenshot 2024-08-18 at 10 03 56 Screenshot 2024-08-18 at 10 09 45
Multiple Screenshot 2024-08-18 at 10 14 16 Screenshot 2024-08-18 at 10 17 01
Occlusion Screenshot 2024-08-18 at 10 04 45 Screenshot 2024-08-18 at 10 13 30

I've updated the changelog with a link to this PR, but didn't mention the breaking change. I'm not sure if that should be included in the changelog or communicated somewhere else.

@jgayfer
Copy link
Owner

jgayfer commented Aug 18, 2024

Makes sense, thanks!

On the breaking change, maybe we should add some other entries to the changelog?

## Changed

- Point lights colours are now added to ambient light, instead of multiplied by it (#24). 

## Migration guide

- Point light intensity needs to be adjusted to account for changes to ambient light. Generally this means point light intensity values need to be lowered. See the relevant changes to the `dungeon` example.

@isidornygren
Copy link
Contributor Author

Makes sense, thanks!

On the breaking change, maybe we should add some other entries to the changelog?

## Changed

- Point lights colours are now added to ambient light, instead of multiplied by it (#24). 

## Migration guide

- Point light intensity needs to be adjusted to account for changes to ambient light. Generally this means point light intensity values need to be lowered. See the relevant changes to the `dungeon` example.

Perfection, I added your suggestion as-is.

Copy link
Owner

@jgayfer jgayfer left a comment

Choose a reason for hiding this comment

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

Looks good. Thank you!

@jgayfer jgayfer merged commit 293cf25 into jgayfer:main Aug 19, 2024
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.

Lights not visible when ambient light is 0 brightness
2 participants