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

[3.2.4] Fix lightmapper build on llvm-mingw on Linux #45285

Merged
merged 1 commit into from
Jan 18, 2021

Conversation

lyuma
Copy link
Contributor

@lyuma lyuma commented Jan 18, 2021

Collectively, these two fixes allow embree to be cross compiled using llvm-mingw on Linux:
Note that in addition to fixing the typo with _MSC_VER, the defined check must be added to avoid undefined symbols acting as 0.
See RenderKit/embree#310

(Originally posted at @JFonS repo at JFonS#4 : It was later partially addressed, but the typo in the _MSC_VER macro was not fixed.)

Might address the issues at #45097 or #45260

Fix typos in #if; add missing _MSC_VER; avoid warning in random generator.
@lyuma lyuma requested a review from akien-mga as a code owner January 18, 2021 14:42
@@ -35,7 +35,7 @@

#if !defined(__aligned)

#if (defined(WIN32) || defined(_WIN32) || defined(__WIN32__) || defined(__NT__)) && !defined(__CYGWIN__)
#if defined(_WIN32) && defined(_MSC_VER)
Copy link
Member

Choose a reason for hiding this comment

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

Heh I'm pretty sure I had commented on the original PR that this was a very convoluted way to check _MSC_VER... looks like it was missed.

@akien-mga
Copy link
Member

Supersedes #45284?

@akien-mga
Copy link
Member

Also fixes the warning reported here: 112b416#r46021572

akien-mga referenced this pull request Jan 18, 2021
Completely re-write the lightmap generation code:
- Follow the general lightmapper code structure from 4.0.
- Use proper path tracing to compute the global illumination.
- Use atlassing to merge all lightmaps into a single texture (done by @RandomShaper)
- Use OpenImageDenoiser to improve the generated lightmaps.
- Take into account alpha transparency in material textures.
- Allow baking environment lighting.
- Add bicubic lightmap filtering.

There is some minor compatibility breakage in some properties and methods
in BakedLightmap, but lightmaps generated in previous engine versions
should work fine out of the box.

The scene importer has been changed to generate `.unwrap_cache` files
next to the imported scene files. These files *SHOULD* be added to any
version control system as they guarantee there won't be differences when
re-importing the scene from other OSes or engine versions.

This work started as a Google Summer of Code project; Was later funded by IMVU for a good amount of progress;
Was then finished and polished by me on my free time.

Co-authored-by: Pedro J. Estébanez <[email protected]>
@lyuma
Copy link
Contributor Author

lyuma commented Jan 18, 2021

The _MSC_VER typos slipped by because they came from upstream (they supposedly already fixed them internally but never uploaded to github). I closed my previous PR.

About the warning, it is still coercing a 32-bit random number into 24 bits of mantissa in the random number generator, so the issue is still there even if the warning is gone... and it's also possible that it causes the random generator to have a slight bias, due to the decimal place shifting.
The slight bias and 24-bit rounding is probably negligible but don't know enough about the algorithm to say for sure...

Copy link
Contributor

@JFonS JFonS 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 to me!

@akien-mga
Copy link
Member

akien-mga commented Jan 18, 2021

About the warning, it is still coercing a 32-bit random number into 24 bits of mantissa in the random number generator, so the issue is still there even if the warning is gone... and it's also possible that it causes the random generator to have a slight bias, due to the decimal place shifting.
The slight bias and 24-bit rounding is probably negligible but don't know enough about the algorithm to say for sure...

That algorithm actually looks like what we used back in early Godot days before adding PCG-minimal (Edit: Actually I checked and no, that was different). Is there a specific reason why you need this algorithm instead of Godot's existing random @JFonS?

@JFonS
Copy link
Contributor

JFonS commented Jan 18, 2021

@akien-mga I just need a thread-safe and fast random number generator. I reused the one from the previous lightmapper but if we have an already implemented alternative it should work just as well.

@akien-mga akien-mga merged commit 3dcb040 into godotengine:3.2 Jan 18, 2021
@akien-mga
Copy link
Member

Thanks!

@lyuma lyuma deleted the lightmapper_cross_compile_fixes branch January 18, 2021 15:47
@akien-mga akien-mga modified the milestones: 3.2, 3.3 Apr 20, 2021
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.

3 participants