Enabling postproduction degrades rendering quality

tb-viktor commented 1 year ago

Describe the bug 📝

When using the OBC.PostproductionRenderer toggling the post production on with renderer.postproduction.enabled = true; and renderer.postproduction.customEffects.outlineEnabled = true; makes the model edges fuzzy and aliased.

Before (false):

After (true):

Reproduction ▶️

No response

Steps to reproduce 🔢

Follow https://top-docs.vercel.app/docs/Tutorials/FragmentIfcLoader to load an IFC file.
Previously loaded IFC file should be used with fragment highlighting following: https://top-docs.vercel.app/docs/Tutorials/FragmentHighlighter.

System Info 💻

Binaries:
    Node: 20.3.0 - C:\Program Files\nodejs\node.EXE
    npm: 9.8.1 - C:\Program Files\nodejs\npm.CMD
  Browsers:
    Edge: Spartan (44.19041.1266.0), Chromium (117.0.2045.47)
    Internet Explorer: 11.0.19041.1566
  npmPackages:
    openbim-components: ^1.1.0 => 1.1.0

Used Package Manager 📦

npm

Error Trace/Logs 📃

No response

Validations ✅

[X] Read the docs.
[X] Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
[X] Make sure this is a IFC.js components issue and not a framework-specific issue. For example, if it's a THREE.js related bug, it should likely be reported to mrdoob/threejs instead.
[X] Check that this is a concrete bug. For Q&A join our Community.
[X] The provided reproduction is a minimal reproducible example of the bug.

NiklasPor commented 10 months ago

Update: Just saw that this is already included in another issue: https://github.com/IFCjs/components/issues/143#issuecomment-1792295335

I can second this, we're getting several rendering artifacts with thePostproductionRenderer:

PostproductionRenderer:

https://github.com/IFCjs/components/assets/13211347/7bca3970-0a1c-4b67-b036-6a03dd1f59ad

SimpleRenderer:

https://github.com/IFCjs/components/assets/13211347/0b31c992-32ae-4e74-a76e-c2af79645f32

pylgrym commented 8 months ago

We have learned, that this is tied to outlines, so we "fixed" it by disabling outlines, however that is a feat in itself.

agviegas commented 4 months ago

After investigating this issue, I've found that the quality of the outlines pass degrades after being resized:

https://github.com/ThatOpen/engine_components/assets/56475338/b126f021-d824-4254-8005-2c5a30c13951

I've fixed it, and shouldn't happen again from version @thatopen/components-front@2.0.14. See this:

https://github.com/ThatOpen/engine_components/assets/56475338/d10707a7-0eb4-463f-a0b4-095515b74298

I suspect that this won't solve the issue described by @NiklasPor, which seem to be due to precission (surfaces being to close to each other, so the renderer is not able to determine which is in front of which).

I'm not sure how to solve this from the library, but one thing you can do is exclude a specific IFC category from being added to the outline computation by filtering them using the classifier (e.g. by IFC category) and then adding them to postproduction.customEffects.excludedMeshes, just like we do to exclude the grid in the tutorial.

pylgrym commented 4 months ago

I have a random shot-in-the-dark tip for the following issue - it might not apply, but you will be able to recognise so quickly.

I suspect that this won't solve the issue described by @NiklasPor https://github.com/NiklasPor, which seem to be due to precission (surfaces being to close to each other, so the renderer is not able to determine which is in front of which).

I'm not sure how to solve this from the library, but one thing you can do is exclude a specific IFC category from being added to the outline computation by filtering them using the classifier https://docs.thatopen.com/Tutorials/Components/Core/Classifier (e.g. by IFC category) and then >adding them to postproduction.customEffects.excludedMeshes, just like we do to exclude the grid in the tutorial.

Is the issue @NiklasPor described the issue commonly known as Z-index fighting?

If so, there is a popular optimization for Z-buffer (which the 'good' renderer might already use?)

A common issue for Z-buffer sorting, is that they map the perspective projection to Z in [-1,1].

There is an optimization, where you reverse this ordering, and offset it to 0/1, so it instead maps [1, 0].

The idea with doing this, is to exploit that floating point numbers have stronger precision around zero, and in the interval [0, 0.5].

The main reason I'm thinking about it, is that we often see lots of Z-index fighting when viewing IFC files, because of their nature - often, the models will contain many thin polygon layers stacked closely.

If this has nothing to do with the issue at hand, please ignore this input :-).

On Thu, 20 Jun 2024 at 13:00, Antonio González Viegas < @.***> wrote:

After investigating this issue, I've found that the quality of the outlines pass degrades after being resized:

https://github.com/ThatOpen/engine_components/assets/56475338/b126f021-d824-4254-8005-2c5a30c13951

I've fixed it, and shouldn't happen again from version @@.*** See this:

https://github.com/ThatOpen/engine_components/assets/56475338/d10707a7-0eb4-463f-a0b4-095515b74298

I suspect that this won't solve the issue described by @NiklasPor https://github.com/NiklasPor, which seem to be due to precission (surfaces being to close to each other, so the renderer is not able to determine which is in front of which).

I'm not sure how to solve this from the library, but one thing you can do is exclude a specific IFC category from being added to the outline computation by filtering them using the classifier https://docs.thatopen.com/Tutorials/Components/Core/Classifier (e.g. by IFC category) and then adding them to postproduction.customEffects.excludedMeshes, just like we do to exclude the grid in the tutorial.

— Reply to this email directly, view it on GitHub https://github.com/ThatOpen/engine_components/issues/128#issuecomment-2180397197, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABVBASI3B2X4T2ZB64VMTR3ZIKY5JAVCNFSM6AAAAAA5SK2VF2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOBQGM4TOMJZG4 . You are receiving this because you commented.Message ID: @.***>

NiklasPor commented 4 months ago

I'll try to check the latest state, sadly we're still running a pre 2.0 version as many of the breaking changes were in conflict with our area-multi-select tool and we have to rewrite it to migrate 😁 I'll get back to this once we're on the latest version again.

ThatOpen / engine_components