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

WIP: Merge noise-info features from VGA internal branch #54

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

sasamiyatu
Copy link
Collaborator

This is a work in progress. Do not merge this yet.

Requesting review to check that the buffers output by the various noise-data options are what are expected. Some may have changed unintentionally, as I had to change the computations somewhat to fit the current internal structure of the path tracing loop.

Saku Haikio and others added 8 commits November 29, 2024 15:04
Original commit:

Author: Saku Haikio <[email protected]>
Date:   Tue Nov 7 12:11:45 2023 +0200

    add material-id, metallic feature and roughness feature
Original commit:

commit 3e3e786c09fe1dc5c178b47931051b88c01ad8a8
Author: Saku Haikio <[email protected]>
Date:   Thu Jan 25 11:34:23 2024 +0200

    add pdf features
@sasamiyatu sasamiyatu requested a review from makitalo December 5, 2024 18:19
Copy link
Collaborator

@makitalo makitalo left a comment

Choose a reason for hiding this comment

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

material-id should be moved from --noise-data to --renderer at some point, and it might be better not to rely on textures for the material index, but those are not critical, this could be merged already. All the features seem to work correctly.

Material and instance-id gbuffers are required only when the
options require them, and the code to read them is injected
with preprocessor macros. Therefore performance should be unaffected
when they are not used.
@sasamiyatu
Copy link
Collaborator Author

I added metallic, roughness and instance-id options to BMFR, these should hopefully work correctly.

The --noise-data-str parameter should probably be separated to at least use a different name for BMFR, as the gbuffer options aren't related to the noise data renderer.

There seems to be some work in progress stuff in the noise data renderer from Saku to allow a maximum of 8 noise data features instead of 4 (using 2 buffers), I could have a look to see if this could be integrated with BMFR. I think some additional testing is needed to make sure the noise data stuff works correctly with BMFR (mainly that the buffers have the expected data in them when used with BMFR).

@makitalo
Copy link
Collaborator

@sasamiyatu thanks, the BMFR output with metallic, roughness and instance-id looks reasonable, so it probably works correctly, but I didn't verify the contents of the buffers.

I agree that --noise-data-str should rather be for example --bmfr.buffers (similar to how SVGF and ReSTIR specify their own parameters).

If you can take a look at the integration of max 8 noise data features, that would be great.

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.

2 participants