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

feat: add Extended Translucency #678

Open
wants to merge 17 commits into
base: dev
Choose a base branch
from

Conversation

ArcEarth
Copy link

Hi, everyone,
I am super excited to contribute to this great project!
Please correct me if I got anything wrong :)
I am a little constraint on time so may reply to things late.

Adding a new feature to control translucent material's opacity.
The core changes is all in the HLSL side, where cpp changes just provide 3 setting variables.

Vanilla:
vanilla
Anisotropic Fabric model:
fabric

Settings UI:
settings

Future work:

  • Allow per geometry to override the settings (Not sure about how to do this right now, using a used flag?)

@ArcEarth ArcEarth changed the title Feat: Extended Translucency feat: Extended Translucency Oct 21, 2024
cbuffer FeatureData : register(b6)
{
GrassLightingSettings grassLightingSettings;
CPMSettings extendedMaterialSettings;
CubemapCreatorSettings cubemapCreatorSettings;
TerraOccSettings terraOccSettings;
LightLimitFixSettings lightLimitFixSettings;
TransclucencySettings transclucencySettings;
Copy link
Author

Choose a reason for hiding this comment

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

The setting buffers seems not working if I put it after wetnessEffectsSettings.
I suspect there is a misalign between the HLSL buffer def and the cpp version of it. But couldn't pin it out so far.

@doodlum
Copy link
Owner

doodlum commented Oct 21, 2024

This looks really interesting. It would be best to include this as an option in the JSON configs for True PBR like we did with the glints PR #413 that way different materials can have different translucency types, rather than every alpha effect getting this. Otherwise random translucent objects could look incorrect which would cause issues. True PBR actually has fabric models too, so could maybe implement that without even needed new config options. You would however need to convert assets to PBR to be able to test the model.

@Jonahex is in charge of all that

Copy link
Owner

@doodlum doodlum left a comment

Choose a reason for hiding this comment

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

Prereq:

Please wait for this issue to be resolved, then this PR can be updated to support it.

@ArcEarth
Copy link
Author

👍 I'll be working on the new nif flags, thanks for the reference!

@ArcEarth ArcEarth force-pushed the extended_transcluency branch 2 times, most recently from c84f52b to 8423cc5 Compare October 27, 2024 07:08
@ArcEarth
Copy link
Author

@doodlum The settings are now detected from ExtraData node in the mesh's NIF.
I find this being much stable than reusing a shading flag.
image

Also, I kept the option to apply this effect to everything, so that existing assets can be improved from this feature immediately.
People can use the setting to make it only apply to the opt-in mesh with the extra data :)
Similarly, mesh can opt-out by setting the extra data to 0.

One future improvement is to make some TESForm based selection like using KID or alkies.

Besides, added another option 'Stregth' to the setting, which is essentially the blending weight (alpha) of this effect.

Copy link
Owner

Choose a reason for hiding this comment

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

This should only add code where !defined (DEFERRED), so anything that's not definitely opaque

Copy link
Author

Choose a reason for hiding this comment

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

Well, I tried to have #if !defined (DEFERRED), but this is causing the effect to go away from one of my equipment >_<
But its still up on another equipment.
Wondering if you know which flag will cause a mesh to appear in deferred pass?

Copy link
Owner

@doodlum doodlum Oct 27, 2024

Choose a reason for hiding this comment

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

This can where we use the extra data for SSS etc. which is a buffer that updates almost every frame anyway. You just need to add a flag to indicate when it definitely needs to update. We may remove this buffer in the future since we are doing too many CB updates a frame.

Copy link
Author

Choose a reason for hiding this comment

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

I end up with a truly constant constant buffers solution:
We have one cbuffer for each material model, and it never needs runtime update: We just assign them into the shader slot. This should avoid most of the cost of synchronized GPU access ;)

@doodlum
Copy link
Owner

doodlum commented Oct 27, 2024

Looks really good so far. Just a few minor performance related things and I think then it's probably good to go.


buffers[0] = materialDynamicCB->CB();
materialDynamicCB->Update(&params, sizeof(params));
context->PSSetConstantBuffers(materialCBIndex, 1, buffers);
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could use hook into Unmap in order to inject parameters into vanilla PerGeometry buffer
https://github.com/doodlum/skyrim-community-shaders/blob/dev/src/Hooks.cpp#L187

Copy link
Author

Choose a reason for hiding this comment

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

I end up with a truly constant constant buffers solution:
We have one cbuffer for each material model, and it never needs runtime update: We just assign them into the shader slot. This should avoid most of the cost of synchronized GPU access ;)

BTW, based on the name of these cbuffer, I thought Skyrim is having one DX constant buffer per-geometry/material based on CRC, or we only have one cbuffer for each shader, and Map it every frame?


ID3D11DeviceContext* context = reinterpret_cast<ID3D11DeviceContext*>(renderer->GetRuntimeData().context);
ID3D11Buffer* buffers[1];
if (auto* data = pass->geometry->GetExtraData(NiExtraDataName)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure how good for perf it to look for ExtraData in each PerGeometry

Copy link
Author

Choose a reason for hiding this comment

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

I think it should be fine compared to the cost of updating constant buffer:
The ExtraData is empty on most geometry, and for others with a small amount of extra data, it's a linear/binary lookup on an array of char*, the compare is pointer based instead of strcmp.

I tried to reuse some shader flags, like VertexAlpha, but found there are lot of random assets using them 😂

src/Buffer.h Outdated Show resolved Hide resolved
@ArcEarth
Copy link
Author

ArcEarth commented Oct 29, 2024

@doodlum @Jonahex @FlayaN
The new update make use of the cbuffer on slot 7 with immutable buffers that never update/map-unmap after creation. And we only have 5 of them in total. This design can avoid the expensive GPU resources update and synchronization and should be very performant. (DX11 will create new GPU buffer when mapped to write, if the current constant buffer is in use with the GPU, which looks like a common case in the codebase: the same constant buffer seems reused for different geometries and mapped to CPU to udpate before the draw)
Wondering if you like the design or prefer merging this into the per-geometry buffer?
And anything else I should address?

@doodlum
Copy link
Owner

doodlum commented Oct 29, 2024

@doodlum @Jonahex @FlayaN
The new update make use of the cbuffer on slot 7 with immutable buffers that never update/map-unmap after creation. And we only have 5 of them in total.
Wondering if you like the design or prefer merging this into the per-geometry buffer?
And anything else I should address?

It is probably fine for now. Soon we will probably scrap all additional constant buffers. State switching and updates need to be minimised but it only matters when it affects a significant number of calls.

So two more things:
Use fake enums in HLSL where possible, like we now do with shader flags.
Link to any references used in the code.

@doodlum
Copy link
Owner

doodlum commented Oct 29, 2024

Please see https://github.com/doodlum/skyrim-community-shaders/blob/dev/package/Shaders/Common/Permutation.hlsli

ArcEarth and others added 12 commits October 29, 2024 20:56
Adding View Dependent Transparency methods to the game.
This effect will make transclucent surface more opaque when view direction is parallel to it.
The effect models regular fabric's transcluceny from the seams between threads.
The thickness of the threads will occlude the seams based on view angle.
- We now use 5 pre defined constant buffers which don't need update
- The only buffer update happens in setting menu
- Minor fixes addressed in review
- #if !defined(DEFERRED) cause the effect to disapper in one of my testing equipment, thus not included.
@ArcEarth ArcEarth force-pushed the extended_transcluency branch from 810a105 to 7d51a1d Compare October 30, 2024 04:40
@ArcEarth
Copy link
Author

@ArcEarth ArcEarth requested a review from doodlum October 31, 2024 04:48
@ArcEarth
Copy link
Author

ArcEarth commented Oct 31, 2024

@doodlum Anything else to be addressed before we can merge? :)

Beside, wondering if we have some detail about the mod release process, if I or the team should upload the feature mod?

Also, the raw Lighting.hlsl edit can be release as a replacer for the current stable version of CS (without settings or nif settings), wondering if we should release it now or release it with the next version?

@ArcEarth ArcEarth requested review from FlayaN and Jonahex November 2, 2024 04:35
@ArcEarth
Copy link
Author

ArcEarth commented Nov 2, 2024

@doodlum @FlayaN @Jonahex Mind to re-review this :)

@doodlum
Copy link
Owner

doodlum commented Nov 4, 2024

@doodlum @FlayaN @Jonahex Mind to re-review this :)

I will just busy atm

}

buffers[0] = materialCB[params.AlphaMode]->CB();
context->PSSetConstantBuffers(materialCBIndex, 1, buffers);
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not a fan of spamming graphics commands, even if it doesn't cause a state switch.

@@ -0,0 +1,46 @@
// ExtendedTranslucency::MaterialParams
cbuffer ExtendedTranslucencyPerGeometry : register(b7)
Copy link
Owner

@doodlum doodlum Nov 8, 2024

Choose a reason for hiding this comment

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

It should use PermutationCB because that updates anyway, during most calls. You can add flags at any time before the end of the pass. It's more maintainable for the project if constant buffers are not scattered all over the place.

// Mods supporting this feature should adjust their alpha value in texture already
// And the texture should be adjusted based on full strength param
MaterialParams params = transcluency->settings;
if (data->GetRTTI() == NiIntegerExtraDataRTTI.get()) {
Copy link
Owner

Choose a reason for hiding this comment

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

we typically use netimmerse_cast. doubt it matters.

@alandtse alandtse changed the title feat: Extended Translucency feat: add Extended Translucency Nov 28, 2024
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.

4 participants