teamwisp / wisprenderer Goto Github PK

The codebase seems to switch styles every now and then. Intellisense function definition generation settings seem to be inconsistent and need to be updated to keep the codebase clean and coherent.

Some examples:

• int foo(const MyObject& bar) {}
• int foo(const MyObject & bar) {}

The same thing applies to functions that contain pointers. The asterisk is not placed in the same location every single time.

Currently creating scene graph nodes at runtime is impossible. This should be made possible by calling init on nodes during runtime.

The current implementation of parenting (in feature_sg_parenting), relies heavily on which nodes are updated first (parent's transform). This means that the current system of nodes updating (grouping them by type, rather than dependency) is not valid anymore. If a camera is linked to a MeshNode, the mesh node will always be updated after the camera. This causes jitters. I'd recommend updating all nodes by dependency, not type.

Instead of ofsetting the virtual addressess use the parameters from DrawIndexed in dx12 instead. The biggest benefit of big buffer ib and vbs is no need to bind more than once.

DXGI ERROR: IDXGISwapChain::Present: The application has not called ResizeBuffers or re-created the SwapChain after a fullscreen or windowed transition. Flip model swapchains (DXGI_SWAP_EFFECT_FLIP_SEQUENTIAL and DXGI_SWAP_EFFECT_FLIP_DISCARD) are required to do so. [ MISCELLANEOUS ERROR #117: ]

Either the style guide or the code should be refactored. Most likely this will be solved using a poll.

Shader reloading would be a great help.
Would also require the reloading of pipelines.

The current frame graph implementation is relatively fast. But lacks the possibility for a lot of optimization.
The redesign I propose is re-implementing it using standard object oriented programming. This would allow for more template magic and constexpr functionality. This design should be bench marked against the current implementation though.

The light_node contains Light m_temp. This is quite ugly code especially if you look at where its used.

Describe the bug
Textures from sun temple, when loaded, return a number of mip level that is of 12, but the dds texture only has proper mip levels created up to a certain level < than 12. After that, black and yellow striped textures are contained in the mip levels, which results in those textures being displayed on screen when that mip level is reached.

To Reproduce

• Load a dds texture from Sun temple as one of the scene materials. (Just the albedo works fine)
• Move the camera further away from the scene and see the striped texture appear on the models
• Another way is also using Nvidia Nsight Graphics and inspecting the resource mip levels.

Expected behaviour
We expect those textures to have proper mip levels, but we have to properly test with different textures as this is not a bug in the mipmapping implementation.

Materials can't be individually removed from the material pool.

Describe the solution you'd like
Being able to remove materials individually.

Is your request related to a problem? Please describe.
A clear and concise description of what the problem is. Ex. I'm always frustrated when [...]

Describe the solution you'd like
A clear and concise description of what you want to happen.

Describe alternatives you've considered
A clear and concise description of any alternative solutions you've considered.

Additional context
Add any other context or screenshots about the request here.

CommandList Execute calls signal and so does the code after it calls Execute.

This requires testing. But the current multi-threading implementation might cause a race condition when a thread tries to wait? Like I said this requires investigation.

Is your feature request related to a problem? Please describe.
Variable roughness with soft shadows and recursions will amp up the ray count dramatically and viewing the number of rays will give insight into how an optimization affects the ray count and how that ray count affects performance. Not only is this good for reflections, this also allows us to see how many rays (total) our program is casting; and which parts we can optimize further.

Describe the solution you'd like
StructuredBuffer counter with InterlockedAdd, to serve as an atomic counter where for every ray you increment. counter[0] is the camera rays (0 for hybrid), counter[1] is the shadow rays, counter[2] is the reflection rays, counter[3] is the AO rays, counter[4] is the GI rays.
This structure can be read from the GPU and can use a shader to render text with the count (to allow triple buffering) or can be read from the CPU and output to the console.

Describe alternatives you've considered
I tried looking into it through nsight, but it doesn't provide any insight into those stats.

Additional context
N.A.

Is your feature request related to a problem? Please describe.
We only support a few file formats. If we want people to use the plug-in, we should support more of the most common file formats such as JPG etc.

Describe the solution you'd like
I'd like to see support for more image formats.

Describe alternatives you've considered
I considered forcing people to use png files everywhere, but that doesn't seem like a good solution.

Additional context
Not applicable.

It seems like the index and vertex buffer's offset is never 0. This is wasted memory.

The current implementation of instancing could be optimized. Currently it always updates the mesh batches and sends it to the GPU, the reason is that setting a pointer on init for MeshNode won't be valid if another MN gets destructed. This could potentially be fixed by moving MeshNodes after the destructed MN back a step, requiring them to be recalculated next frame. This same issue is present with the current light system.

All optimizations & changes for draw calls include:

• Include instancing into vertex buffers (Con: requires changes to root signatures, it is less easy to add per object data and matrices can't be sampled directly, Pro: Is faster)

• Instancing is limited to n instances where n = 48 * 1024 / 80, because buffers bigger than 48 KiB get slow. Of course this hard limit can be changed, but is discouraged for speed.

• When a batch is out of space it doesn't allocate a new batch, causing a cut off after 614 instances.

• SceneGraph::Optimize takes too much time, MeshNodes should have an ObjectData* that can change location depending on if MeshNodes before it have been deallocated (it should be one filled array of ObjectData).

• (Scope increaser): Multi Draw Indirect could be used to batch up all kinds of geometry, which would require VBO and IBO to allow suballocating meshes into it

• (Scope increaser): Parts of geometry could be culled with a compute shader (or on CPU), MDI with this technique would be a lot of performance gain.

Optimizations & changes for lighting include:

• Setting up lights takes too much time, LightNodes should have a Light* that can can change location depending on if LightNodes before it have been deallocated (it should be one filled array of Light).

• (Scope increaser): Lights could be culled with either CPU or compute shader

• (Scope increaser): Clustered Deferred Rendering severely decreases the number of light calculations

Vertex buffers and index buffers are not required to be 64kb aligned since they are not part of a heap in this case. (This need to be checked though)

Is your feature request related to a problem? Please describe.
There's no problem whatsoever, however, I'd like to link people to a repository that has a sensible name. This is currently not the case.

Describe the solution you'd like
I'd like to see the name of this repository changed to something that matches the project a lot better.

Describe alternatives you've considered
Not applicable.

Additional context
I'd like to start posting to Reddit and other forums as well, and it doesn't make sense to link to the repository right now.

Is your design request related to a problem? Please describe.
Inconsistency in design

Describe the solution you'd like
Everything just should use radians. Makes it compatible with DXmath better.

Additional context
Transformations and camera.

Model pool should log some thing if it runs out of memory for example and for the other error cases.

*Description
The skybox in 2 renderers is rotated 90 degrees from the third renderer. Not sure which one is the correct one. This might be caused by the different coordinate systems.

To Reproduce
Steps to reproduce the behavior:

1. Look at the skybox.
2. Switch between frame graphs (F2).

Expected behavior
The skybox should be displayed like it is in every other frame graph.

Desktop (please complete the following information):

• OS: Windows 10
• Version: Not relevant
• Hardware: Not relevant

Some nested if statement are unnecesarry and wastefull. May be more frequent in d3d12_renderer.cpp

if constexpr (d3d12::settings::use_exec_indirect)
{...}
else
{
if constexpr (!d3d12::settings::use_exec_indirect)
{...}
}

Currently flat out ignores the templates given at the moment. They are there for a reason and should be used. We need to be able to load models with any format. Maybe I don't want normals for this model.

Is your request related to a problem? Please describe.
If a submodule gets a update that breaks compatibility it would break our renderer at the moment. This should be prevented.

Describe the solution you'd like
The repository settings should be updated to clone a specific commit ID.

Describe alternatives you've considered
Creating forks could be a option but unnecessary if we don't make changes.

Additional context
This is not something we want to do until late in the development process. We want to use the latest versions of all libraries. If it breaks our renderer we can immediately fix it. When we release we should make this change.

Is your feature request related to a problem? Please describe.
The current readme is boring and doesn't let people know what our renderer is capable of.

Describe the solution you'd like
I'd like to see screenshots and a short description of the project. As well as some kind of installation manual.

Describe alternatives you've considered
Linking to our Wiki is not convenient for users.

Additional context
I'd like to post links to the repository on forums (relevant issue: #203), so this change would be very much appreciated. I'd do it myself if I had the time for it...

DebugLayer in d3d12_settings.hpp uses 'enum' instead of 'enum class' resulting into ENABLE, DISABLE and ENABLE_WITH_GPU_VALIDATION to be available to everything in wr::d3d12::settings, without specifying DebugLayer. Could this be present elsewhere?

Is your design request related to a problem? Please describe.
When there are multiple model pools, execute indirect will not work properly (or even crash). Bind index buffer is called for every mesh, while this should be up to the execute indirect to bind the correct index buffer(s). This causes the execute indirect to be carried out using the latest index buffer bound; batches[end0].m_meshes[end1];

Describe the solution you'd like
Add index buffer view to IndirectCommandIndexed

Describe alternatives you've considered
N.A.

Additional context
N.A.

Is your design request related to a problem? Please describe.
The intermediate data of model loader copies data twice and the copying doesn't use memcpy but uses operator=. It also uses vectors of attributes and not vectors of structs, meaning that you can't memcpy them over but have to swap them around again. This increases load time and also means the same data is duplicated while it doesn't have to.

Describe the solution you'd like
Use memcpy where needed, change assimp wrapper to allow direct copying.

Describe alternatives you've considered
N.A. This doesn't have to be fixed, but it will decrease load times.

Additional context
N.A.

Describe the bug
The program crashes when shut down while running in DEBUG, returning code 3 instead of 0 and giving the following error:
Debug Assertion Failed! Program: ...cedural-Ray-Tracing\build_vs2017_win64\bin\Debug\WispDemo.exe File: minkernel\crts\ucrt\src\appcrt\heap\debug_heap.cpp Line: 908 Expression: is_block_type_valid(header->_block_use) For information on how your program can cause an assertion failure, see the Visual C++ documentation on asserts.
To Reproduce
Steps to reproduce the behavior:

1. Run the project
2. Click the close button
3. See error

Expected behavior
The program should shut down properly, returning code 0.

Desktop (please complete the following information):

• OS: Win 10
• Version 1809
• Hardware CPU: 6700HQ, GPU: GTX 970M, RAM: 16GB (Koen)
• Hardware CPU: 6700HQ, GPU: GTX 980M, RAM: 32GB (Florian Laptop)
• Hardware CPU: 8700K, GPU: GTX 1080, RAM: 16GB (Florian Desktop)

Currently we do the following:

// Platform independend
struct CommandList
{
}

struct D3D12ComamndList : CommandList, d3d12::CommandList {};

struct d3d12::CommandList
{
// DX12 implementmation
}

This could be reweritten as:

using CommandList = void*;

Is your request related to a problem? Please describe.
D3D12RenderSystem class can currently be inherited from but marks all virtual functions as final giving no use to the child class. maintaining D3D12RenderSystem when the RenderSystem class changes can introduce mistakes as it is not clear that D3D12RenderSystem is not supposed to be inherited from in current design philosophy.

Describe the solution you'd like
D3D12RenderSystem class marked with the final specifier and remove in necessary final specifiers from functions. this makes all future additions to RenderSystem when virtual to be final in the D3D12RenderSystem.

Describe alternatives you've considered
none

Additional context
none

When using SampleLevel instead of the [] operator in the compute shader for deferred compostion weird aliasing effects happen.

Describe the bug
Resizing the engine window causes memory to leak.

To Reproduce
Steps to reproduce the behaviour:

1. Run the project in Visual Studio
2. Click on the corner of the window and keep resizing it
3. Keep an eye on the Process Memory graph in the Diagnostic Tool
4. Notice how the memory goes up, but never gets freed.

Expected behaviour
If memory goes up because something needs to be allocated, the expected behaviour is that then, after the resizing is done, it should go back down to the level it was before.

Describe the bug
Starting the renderer on RT_HYBRID or FULL_RAYTRACING pipelines instead of DEFERRED, makes the engine crash.

To Reproduce

1. Go to demo_frame_graphs.hpp
2. Change this line

static PrebuildFrameGraph current = fg_manager::PrebuildFrameGraph::DEFERRED;

Into

static PrebuildFrameGraph current = fg_manager::PrebuildFrameGraph::RT_HYBRID;

3. Watch it crash.

Expected behaviour
The engine should just run normally and render using the desired pipeline.

Screenshots
None.

Is your design request related to a problem? Please describe.
It causes confusion and errors when setting up new render tasks

If you set a material constant and don't set a texture the renderer will crash on initialization.

Describe the bug
Wrong materials may cause graphical errors in raytracing.

To Reproduce
Run the application 100 times and it may break.

Describe the bug
On my last gen 980m I can't run the latest shader model. In debug it works fine as it goes down to a previous shader model however when running in release the demo crashes as the shader model used isn't supported.

To Reproduce
Set Solution configuration to Release on a 980m and run the demo

Expected behaviour
A clear and concise description of what you expected to happen.
The program will assert and give a DX error message in the output exclaming the shader model is not supported.

Screenshots
If applicable, add screenshots to help explain your problem.

Desktop (please complete the following information):

• Hardware CPU:6700HQ, GPU:GTX 980m, RAM:32GB]

Additional context
Add any other context about the problem here.

Describe the bug
Only the current frame graph resizes the framebuffers on resize. The others will not be resized.

To Reproduce
Steps to reproduce the behavior:

1. Resize the window
2. Switch frame graph (F2)

Expected behavior
All framebuffers should be resized.

Screenshots
If applicable, add screenshots to help explain your problem.

Desktop (please complete the following information):

• OS: Windows 10
• Version: Not relevant
• Hardware: Not relevant

Describe the bug
When closing the application, when the texture pool gets destroyed a read access violation (location: 0xFFFFFFFFFFFFFFFF) is thrown in the GetFrameIdx function.

To Reproduce
Steps to reproduce the behaviour:

1. Run the application
2. Close it after initialization is finished
3. See error

Expected behaviour
The application should shut down without any errors.

Desktop (please complete the following information):

• OS: Win 10
• Version 1809
• Hardware [e.g. CPU:6700HQ, GPU:GTX 970M, RAM:16GB]

Describe the bug
The deferred composition task uses a RGB8 render target. This is not what we want for HDR support.

To Reproduce
d3d12_deferred_composition.hpp/cpp

Expected behaviour
We want the buffer to be 32RGB so we can output it as hdr after post processing.

Is your design request related to a problem? Please describe.
The current implementation of parenting only uses euler angles that get converted to quaternions and then to matrices. This is correct, but doesn't make any sense. It would be better to convert "all" matrices to quaternions; reducing the memory/bandwidth cost and increasing performance.

Describe the solution you'd like
Only use matrices when needed (like clip space conversions) and use quaternions otherwise.

Describe alternatives you've considered
Matrices can still be used, but will result into rotations being less accurate and potential rendering issues.

Additional context
N.A.

Currently render targets are not versioned. This results in flickering at very low frame rates and unnecessary CPU stalls.

node.hpp specifies std::bitset<3> (std::vector<3> before, but changed for performance reasons). This means that when a different type of buffering is used (double or quadruple) the required_update field is accessed the wrong way.

Is your design request related to a problem? Please describe.
I am always annoyed that I expect a handle to be just copied over and then some function or data class requires a pointer to that handle for long term storage.

issue present at line 50: model_pool.hpp, struct Model, std::vector<std::pair<Mesh*, MaterialHandle* >> m_meshes;

Describe the solution you'd like
The design of a handle should allow it to be copied around without issues as if it is an advanced pointer. handles should never be passed of stored as pointers.

Execute indirect will only render every third frame. Likely to be related to versioning.

Member variables width and height in RenderTargetProperties on lines 17 and 18 are not prefixed with m_

Describe the bug
Scene graph never deletes batch if last MeshNode with batched mesh is destroyed. Renderer crashes on a pointer to mesh that does not point to anything valid if new node with same name gets added

To Reproduce
Steps to reproduce the behaviour:

1. add MeshNode to scenegraph
2. delete that node
3. added Node. make sure it has the same name
4. See error

Expected behaviour
to re use the batch or remove the old batch when creating a new one.