GPUOpen-Effects / TressFX

DirectX 12 and Vulkan libraries that provide convenient access to realistically rendered and simulated hair and fur
MIT License
766 stars 129 forks source link

store positions relative to the mesh surface #6

Closed mrgreywater closed 8 years ago

mrgreywater commented 8 years ago

tl;dr: Store positions relative to the mesh surface to avoid clipping when the simulation is stopped or skipping frames.

This fixes the mesh mesh clipping through the hair as seen here:

Mesh Clipping

Explanation:

Before, it worked the following way:

With this PR, it works like this:

Sidenote: Reducing the simulations speed makes the artifacts this PR fixes more obvious. You can now disable the simulation, while the animation still runs.

Alternatives:

  1. Interpolate output-positions (not the positions used in the simulation) in GenerateTransforms. Pros: Easy to implement. Cons: Need to iterate vertices for each strand in GenerateTransforms; Vertexshader is probably faster.
  2. Simulate each frame, even when over 60FPS. This requires time-corrected verlet integration, and probably some other tweaks. Pros: No additional Transforms needed. Cons: Simulation must be executed each frame and cannot be disabled. (You can try the time-corrected TressFX version here: https://github.com/mrgreywater/TressFX/tree/time-corrected-verlet). The time-corrected verlet integration should also be used anyways, since the simulation is not executed with fixed step. If you try the linked build you will see that it looks both more realistsic and needs far less length constraint iterations before it converges to a stable result. Using TCV and this PR is not mutual exclusive.

Final note: I am aware of the fact that this change was not discussed properly in the issue section as is recommended. So in case we find a better solution to this problem, i'm ok if this PR is rejected.

mrgreywater commented 8 years ago

ping @jstewart-amd

jstewart-amd commented 8 years ago

As mentioned in the corresponding issue, "I'll be in full GDC scramble mode." This is the busiest time of the year for me. I am going to have to ask for your continued patience. I do hope to talk to our TressFX simulation guru about this today, though.

mrgreywater commented 8 years ago

@jstewart-amd After GDC, Easter, GTC and over a month of waiting, can please somebody take a look at this (and the various issues that don't have a single reply). I find it kind of unbelievable that a big company like AMD only has one busy employee that looks over all the GPUOpen repositories. Communication and seeing active development is important for any open source project, and I believe having long delays like these will put off other developers that may have contributed otherwise.

jstewart-amd commented 8 years ago

Communication and seeing active development is important for any open source project, and I believe having long delays like these will put off other developers that may have contributed otherwise.

I agree with you, for what that's worth.

Your PR title says that it is work in progress and should be discussed before merging. What specifically do you want to discuss? I've tested the changes locally, and it is an obvious visual improvement for the bear model, but at the cost of some additional simulation time. Given the care you took with preparing and documenting your PR and how long you've had to wait, my inclination is just to merge your PR as is and put whatever work we feel is still needed at the top of the backlog. We already know we need to take another optimization pass over all the code, for example. The "yeah, but it makes the simulation more expensive" aspect of your PR can just be lumped under the already existing and high-priority to-do of "make everything faster".

mrgreywater commented 8 years ago

Thanks for the reply. Since the general direction of this PR seems to be alright, I'll go ahead and implement the first todo item:

Do the same transformations when there is only a single-head-matrix.

before the merge.

mrgreywater commented 8 years ago

Did the same thing for the single head transforms. When translating the head, the hair will not clip through the head as it did before. Also you can disable the simulation now and still translate the model.

The whole thing should still be optimized in the future, but it fixes the problematic clipping issues for now.

jstewart-amd commented 8 years ago

Thanks, @mrgreywater

Assuming no show-stopping issues, I'll get this merged today or tomorrow.

jstewart-amd commented 8 years ago

This is merged now. Thanks, @mrgreywater

There is now a new warning from the D3D debug runtime: D3D11 ERROR: ID3D11DeviceContext::CSSetUnorderedAccessViews: On devices created at a Feature Level D3D_FEATURE_LEVEL_11_0, the maximum Compute Shader Unordered Access View slot is 7 (Offset 0 and NumViews 9 specified). [ STATE_SETTING ERROR #2097404: DEVICE_CSSETUNORDEREDACCESSVIEWS_TOOMANYVIEWS]

In the short term, I will probably just disable the warning with code like this:

#ifndef NDEBUG
    if( SUCCEEDED( hr ) )
    {
        ID3D11Debug * d3dDebug = nullptr;
        if( SUCCEEDED( pd3d11Device->QueryInterface(IID_PPV_ARGS(&d3dDebug) ) ) )
        {
            ID3D11InfoQueue* infoQueue = nullptr;
            if( SUCCEEDED( d3dDebug->QueryInterface( IID_PPV_ARGS(&infoQueue) ) ) )
            {
                // ignore some "expected" errors
                D3D11_MESSAGE_ID denied [] =
                {
                    D3D11_MESSAGE_ID_DEVICE_CSSETUNORDEREDACCESSVIEWS_TOOMANYVIEWS,
                };

                D3D11_INFO_QUEUE_FILTER filter;
                memset( &filter, 0, sizeof(filter) );
                filter.DenyList.NumIDs = _countof(denied);
                filter.DenyList.pIDList = denied;
                infoQueue->AddStorageFilterEntries( &filter );
                infoQueue->Release();
            }
            d3dDebug->Release();
        }
    }
#endif

In the longer term, I'm assuming the number of UAVs will drop when we do our optimization pass.

mrgreywater commented 8 years ago

tbh we don't need the ninth uav slot anyways, I just checked. I will send a small fix tomorrow. Thanks for the heads-up.