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

Use C++11 random number generator for fuzzy skin #5682

Merged
merged 2 commits into from
Jun 15, 2024

Conversation

SeaRyanC
Copy link
Contributor

Description

Fixes #5521
Fixes bambulab/BambuStudio#4253

This is an alternate, probably better choice, to #5659. The root cause analysis is the same so you can read that PR description for more info.

The current implementation in this function is pretty flawed. In runtimes that use thread-local RNG state (Windows), you get problems like #5521 where each thread produces the same output, leading to obvious artifacts whenever a series of layers have identical geometry. It's awkward to get a per-thread initial seed without a true entropy source, as #5659 shows. I also don't like how that PR is technically capable of calling srand in non-thread-local runtimes through multiple threads.

In runtimes that don't use thread-local RNG state, the situation is arguably worse. rand and srand are not thread-safe, so there's possible state corruption -- I don't have any evidence, but we do have reports of AVs specific to fuzzy skin, and this could be a root cause. These functions get called a lot here and any edge case is going to get hit sooner or later.

Since don't really know in-code whether our runtime has thread-local state or not, it's not safe to call srand (might corrupt state in global-state runtimes), and it's not safe to not call srand (might get layer patterning in thread-local runtimes). The STL primitives here are an easy solution since they're instanced and we can simply make one instance per thread.

Note that std::random_device is allowed to "generate the same number sequence" if a hardware entropy source is not available. In that case .entropy() returns 0 and we'll fall back to the thread ID hash method instead.

Screenshots/Recordings/Graphs

See images in #5659 for the defect cases.

To compare the old PRNG with the new one, here's a Stanford bunny, cylinder, cube, and sphere, with the default fuzzy skin setting applied. Apologies for not getting the camera lined up exactly the same in both shots but it's reasonably easy to tell that there's not an observable difference.

Current behavior in Linux (main branch)
bunny - main

With this PR
bunny - stl

Tests

Tested on Linux

Copy link
Owner

@SoftFever SoftFever left a comment

Choose a reason for hiding this comment

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

I tested it.
It works like a charm.
Thank you for fixing this issue!

@SoftFever SoftFever merged commit b2a5543 into SoftFever:main Jun 15, 2024
12 checks passed
@L2on1 L2on1 mentioned this pull request Jul 24, 2024
3 tasks
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.

Fuzzy skin prints same noise profile on consecutive layers Fuzzy skin slicing with identical patterns
2 participants