blurrypiano / littleVulkanEngine

Code repo for video tutorial series teaching Vulkan and computer graphics
MIT License
829 stars 147 forks source link

Project Feedback [enhancements | suggestions | wishlist] #4

Open HugoPeters1024 opened 3 years ago

HugoPeters1024 commented 3 years ago

At numerous occasions in the tutorial you made it clear that it is important to delete the copy constructors. I find that using some sort of NoCopy class to inherit from is more convenient than doing it manually:

class NoCopy
{
public:
    NoCopy(NoCopy const&) = delete;
    NoCopy& operator=(NoCopy const&) = delete;
    NoCopy() = default;
};

For any other class you can simply inherit privately:

class LveDevice : NoCopy {
...
}

Whether or not you find this helpful, thanks so much for the high quality videos <3

MythreyaK commented 3 years ago

Might also add the final keyword, compiler magic can do more optimizations :)

blurrypiano commented 3 years ago

Both of these are great suggestions. It was careless of me not to include this from the start, downsides to being the one to review your own code 😅

I've been having trouble thinking of how to integrate NoCopy into the series, since I think always using manual deletion is preferable to using a mix of NoCopy and manual.

So far what I'm thinking is that around tutorial 25 when I've finished covering the basics, I'll do an epsiode going through all the mistakes, questionable design decisions, etc. and it's in that video I can cover the NoCopy change, and marking things final., but I'm open to suggestions.

HugoPeters1024 commented 3 years ago

I see where you're coming from, that sounds like a reasonable idea :)

MythreyaK commented 3 years ago

I'll do an episode going through all the mistakes, questionable design decisions, etc

Yep that'd be great! I'm having some trouble coming up with an extensible way of architecting the rendering framework, and it feels a bit like spaghetti at this point. I realize this can be a big book or a whole series in its own right, but perhaps an overview of how you'd do it, and how it'd be extensible to allow for varied pipeline, renderpass setups would help me out a ton!

dejaime commented 3 years ago

Maybe an [enhancements | suggestions | wishlist] issue to keep all of them in one place could be good, as it will probably take a while before they can be tackled. This would help reducing open issues while also incentivizing feedback.

IvaKom commented 3 years ago

Great tutorials! I am making my way through, refreshing my understanding of Vulkan. I find it a delight to use designated struct initializers when dealing with numerous VkXxxYyyInfos. With them, instead of

VkApplicationInfo appInfo = {};
  appInfo.sType = VK_STRUCTURE_TYPE_APPLICATION_INFO;
  appInfo.pApplicationName = "LittleVulkanEngine App";
  appInfo.applicationVersion = VK_MAKE_VERSION(1, 0, 0);
  appInfo.pEngineName = "No Engine";
  appInfo.engineVersion = VK_MAKE_VERSION(1, 0, 0);
  appInfo.apiVersion = VK_API_VERSION_1_0;

we get

VkApplicationInfo appInfo {
  .sType = VK_STRUCTURE_TYPE_APPLICATION_INFO,
  .pApplicationName = "LittleVulkanEngine App",
  .applicationVersion = VK_MAKE_VERSION(1, 0, 0),
  .pEngineName = "No Engine",
  .engineVersion = VK_MAKE_VERSION(1, 0, 0),
  .apiVersion = VK_API_VERSION_1_0
};

Which are less verbose, self-contained and encourage keeping all the initialization in one place. Using them would require upgrading to C++20 standard, however.

AlexJustRandom commented 2 years ago

You've talked about many times that you consider the ECS a future.Do you plan to use ECS library like entt in the project? or creating small ECS implementation for learning purpose? and after that switching to lib. ? Im asking about using lib because ecs is big concept on its own.

Radseq commented 2 years ago

IvaKom, its done, see pull request

blurrypiano commented 2 years ago

@AlexJustRandom - haven't decided 100% yet, but ya your thoughts mirror my own, there's a lot to consider. Will think more on this once we get closer to actually needing to commit

chickenSandwich commented 2 years ago

@blurrypiano - I would be very excited if you were going to review shadow mapping. Do you think you might work this into your tutorials?

blurrypiano commented 2 years ago

@chickenSandwich - yup this is definitely something I will be covering once we tackle multiple render passes