-
Notifications
You must be signed in to change notification settings - Fork 33
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
Render Graph improvements #383
Conversation
a2b30d3
to
b6af393
Compare
0c16b43
to
01a1dbe
Compare
Btw I can't seei ImGui. Not on Windows and neither on Ubuntu. I will debug this using RenderDoc. |
cbdfba2
to
5591ecf
Compare
1465098
to
bdf97ca
Compare
6d92dc0
to
60be537
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2021-05-17 00:48:57.078338 debug 11393 [vulkan-renderer] Running Application.
2021-05-17 00:48:57.079727 debug 11393 [vulkan-renderer] FPS: 0, window size: 800 x 800.
2021-05-17 00:48:57.083832 error 11393 [vulkan-renderer] [ VUID-vkResetCommandPool-commandPool-00040 ] Object: 0x55f30a994950 (Type = 6) | Attempt to reset command pool with VkCommandBuffer 0x55f30a994950[] which is in use. The Vulkan spec states: All VkCommandBuffer objects allocated from commandPool must not be in the pending state (https://www.khronos.org/registry/vulkan/specs/1.1-extensions/html/vkspec.html#VUID-vkResetCommandPool-commandPool-00040)
2021-05-17 00:48:57.083853 critical 11393 [vulkan-renderer] Command line argument --stop-on-validation-message is enabled.
2021-05-17 00:48:57.083857 critical 11393 [vulkan-renderer] Application will cause a break point now!
Process finished with exit code 134 (interrupted by signal 6: SIGABRT)
caused by
vulkan-renderer/src/vulkan-renderer/application.cpp
Lines 310 to 320 in 60be537
// Check if --stop-on-validation-message is enabled. | |
if (user_data != nullptr) { | |
// This feature stops command lines from overflooding with messages in case many validation | |
// layer messages are reported in a short amount of time. | |
spdlog::critical("Command line argument --stop-on-validation-message is enabled."); | |
spdlog::critical("Application will cause a break point now!"); | |
// Wait for spdlog to shut down before aborting. | |
spdlog::shutdown(); | |
std::abort(); | |
} |
even though no argument was passed. Could not test because of this, also I don't understand at most points what the code should do (on a conceptual level), thus I don't feel fit to give a (full) review, but still gladly test it and read through the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic work!
60be537
to
cf87ca0
Compare
It was silly to let this be in an invalid state anyway.
f91b719
to
2519ddd
Compare
@@ -42,7 +42,7 @@ class Application : public VulkanRenderer { | |||
void load_toml_configuration_file(const std::string &file_name); | |||
void load_textures(); | |||
void load_shaders(); | |||
void load_octree_geometry(); | |||
void load_octree_geometry(bool initial); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
void load_octree_geometry(bool initial); | |
/// @param initialize Initialize worlds with a fixed seed. | |
void load_octree_geometry(bool initialize); |
src/vulkan-renderer/application.cpp
Outdated
@@ -183,24 +184,15 @@ void Application::load_shaders() { | |||
spdlog::debug("Loading shaders finished."); | |||
} | |||
|
|||
void Application::load_octree_geometry() { | |||
void Application::load_octree_geometry(bool initial) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename variable.
Since we're storing them now anyway, this makes it look a lot ncier.
* Inline alloc_command_buffers function * Remove maps used to assign a render stage/resource to a physical stage/resource. We now store a `unique_ptr<PhysicalStage>` in `RenderStage` and a `shared_ptr<PhysicalResource>` in `RenderResource`. The reason the latter is shared is because, in the future, multiple render resources will point to the same physical resource. * Store separate vectors of BufferResource and TextureResourceo to allow for faster iteration
So we can see the beautiful octree :)
2519ddd
to
cead7e5
Compare
BufferResource::upload_data()
to allow for dynamically changing data at runtimeshared_ptr
andunique_ptr
)This PR adds support to the render graph for dynamic buffers. This allows for changeable octree geometry at runtime, which a small demo has been created for. ImGUI support has also been rewritten to work with the render graph. It now fully uses the render graph for the two buffers and stage, however it still relies on textures being done externally. This brings around a 33% FPS improvement - and that's with the bigger octree. With the original octree, I get an FPS improvement of about 73%.
In terms of the internals of the render graph, the two
std::unordered_map
s used to map resources&stages to physical resources&stages has been removed. ARenderResource
now holds ashared_ptr<PhysicalResource>
and aRenderStage
now holds aunique_ptr<PhysicalStage>
. The reason aRenderResource
holds a shared pointer is because, in the future, multiple render resources will be able to point to the same physical resource. This makes accessing the physical resource much easier, and removes the need for the uglycreate
functions.Also, instead of storing just a
std::vector<std::unique_ptr<RenderResource>>
, each different type of resource (BufferResource
,TextureResource
, etc.) now has its own vector. This makes it faster to iterate each type of resource.