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

Clarify that LightmapGI is not supported in compatibility renderer #88406

Closed
wants to merge 1 commit into from

Conversation

vaartis
Copy link
Contributor

@vaartis vaartis commented Feb 16, 2024

The documentation currently does not mention that LightmapGI rendering is not supported in the compatibility renderer. It only states the fact that baking it is not supported (thus it is possible to make an assumption that rendering it is supported, while baking isn't).

I'd originally wanted to reply in #75726 (comment) but appening this to documentation seemed fairly easy to do myself.

@vaartis vaartis requested a review from a team as a code owner February 16, 2024 15:40
@vaartis vaartis changed the title Clarify that LightmapGI is not supported in compatibility renderer (at all) Clarify that LightmapGI is not supported in compatibility renderer Feb 16, 2024
@AThousandShips AThousandShips requested a review from a team February 16, 2024 15:44
@AThousandShips AThousandShips added this to the 4.x milestone Feb 16, 2024
Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

Good to know, to not waste the developer's time testing

doc/classes/LightmapGI.xml Outdated Show resolved Hide resolved
@dsnopek
Copy link
Contributor

dsnopek commented Feb 16, 2024

LightmapGI rendering does work with the compatibility renderer - this was added in PR #85120

EDIT: Although, I guess that is only 4.3+, which, of course, isn't released yet. If we're only targeting the docs for 4.2 and below, then it's true that it isn't supported in those versions.

@DarioSamo
Copy link
Contributor

DarioSamo commented Feb 16, 2024

LightmapGI rendering does work with the compatibility renderer - this was added in PR #85120

EDIT: Although, I guess that is only 4.3+, which, of course, isn't released yet. If we're only targeting the docs for 4.2 and below, then it's true that it isn't supported in those versions.

And baking also works since 4.3+ too, the host system just needs to be able to run Vulkan at the moment even if you are in Compatibility mode. I feel the documentation should reflect the current state of the feature.

EDIT: In that case it doesn't seem like a documentation change would be necessary? The original version reflects how it currently works after all.

@Mickeon
Copy link
Contributor

Mickeon commented Feb 16, 2024

Pretend I retracted my approval until I figure how to do so. The documentation should be changed accordingly.

Unless this PR is allowed to _specifically target 4.2 but that would be a major, major exception.

@DarioSamo
Copy link
Contributor

DarioSamo commented Feb 16, 2024

Yeah upon looking at the history, this would seem to overwrite what efb1cba does, which is Clay adding specifically that the feature currently works if you can run a Vulkan renderer even while in compatibility mode. Since #87340 has been merged that statement should be true.

Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

See above (yippie).

@vaartis
Copy link
Contributor Author

vaartis commented Feb 17, 2024

It would still be nice if this was at least in 4.2.2, if possible. Which I think is still yet to have a release and will not have LightmapGI.

@Calinou
Copy link
Member

Calinou commented Feb 17, 2024

It would still be nice if this was at least in 4.2.2, if possible. Which I think is still yet to have a release and will not have LightmapGI.

In this case, you need to open a PR against the 4.2 branch as the statement will only be true for 4.2.x, not 4.3.

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.

None yet

6 participants