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

[renderer] Small refactorings #241

Closed
wants to merge 5 commits into from

Conversation

IAmNotHanni
Copy link
Member

Closes #238.

@IAmNotHanni IAmNotHanni added the cat:refactor refactor/clean up/simplifications/etc. label Sep 8, 2020
@IAmNotHanni IAmNotHanni self-assigned this Sep 8, 2020
@IAmNotHanni
Copy link
Member Author

IAmNotHanni commented Sep 8, 2020

This code change also fixes an issues with queue management. The problem is that sometimes we passed the graphics queue to a constructor which names it internally as data_transfer_queue. Because both are just VkQueue, the constructor accepts it and passes it on even further down to the members.

For example this:

m_textures.emplace_back(*m_vkdevice, m_vkdevice->physical_device(),
                                       m_vkdevice->allocator(), texture_file,
                                       texture_name, m_vkdevice->graphics_queue(),
                                       m_vkdevice->graphics_queue_family_index());

will be passed on to:

Texture::Texture(const wrapper::Device &device, const VkPhysicalDevice graphics_card,
                 const VmaAllocator vma_allocator, const std::string &file_name,
                 const std::string &name, const VkQueue data_transfer_queue,
                 const std::uint32_t data_transfer_queue_family_index)

This new code has been changed so it is the constructor call to CommandBuffer which specifies the queue which is used.

Copy link
Member

@yeetari yeetari left a comment

Choose a reason for hiding this comment

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

Nice work :)

include/inexor/vulkan-renderer/renderer.hpp Outdated Show resolved Hide resolved
include/inexor/vulkan-renderer/wrapper/command_pool.hpp Outdated Show resolved Hide resolved
include/inexor/vulkan-renderer/wrapper/framebuffer.hpp Outdated Show resolved Hide resolved
@IAmNotHanni
Copy link
Member Author

I will include other small refactorings into this pull request as well.

void load_toml_configuration_file(const std::string &file_name);

/// @brief
VkResult load_textures();
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not done yet here but I wanted to push just already. Will finish and squash later.

@IAmNotHanni IAmNotHanni changed the title [renderer] Don't pass data which is already available through device … [renderer] Small refactorings Sep 12, 2020
@IAmNotHanni
Copy link
Member Author

I changed my mind: I overloaded this pull request!
I will git cherry-pick the first 2 commits which contain the changes and your requested fixed, squash them and create a separate pull request.
Then I will do the other issues one by one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat:refactor refactor/clean up/simplifications/etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Do not pass data which is already contained in wrapper::Device
2 participants