search

natinusala / borealis

Hardware accelerated, controller and TV oriented UI library for PC and Nintendo Switch (libnx)
Apache License 2.0
267 stars 84 forks source link

Rule of five for Image class managing a resource. #35

Closed jon-dez closed 4 years ago

jon-dez commented 4 years ago

Add copy and move constructors, as well as copy and move assignment operators to the Image class. This extends the functionality of the Image class so that it could be used directly with STL containers without having to add a layer of pointer indirection.

jon-dez commented 4 years ago

Commit 0db47da is what I am referring to.

natinusala commented 4 years ago

Thanks for the PR!

That is a welcome change but may I ask in what use case do you need to copy / move views? I don't know if it's something I'm willing to support

Also why are there commits from your other PR? :thinking:

jon-dez commented 4 years ago

I agree there is no need for views to be copied or viewed, at least there is no reason that I am aware of. But the change makes more sense when the Image is thought of more as a resource instead of a view. When it is treated as a resource a group of Images could be loaded into a container on a seperate thread so the main UI thread isn't blocked. Then when the images are finally loaded the images could be moved from one container to another and then finally displayed.

What I explained could be done using the example containers for the images:

std::vector<brls::Image*> related_imgs_0;

std::vector<brls::Image> related_imgs_1;

But when related_imgs_0 is being used, the actual memory associated to the related set of images aren't guaranteed to be in contiguous memory. Thus, related sets of images are now in unrelated areas in memory.

In related_imgs_1, the memory associated to the related set of images are always guaranteed to be in contiguous memory. In turn, related sets of images are within the bounds of the vector allowing for faster memory access to sets of images that are to be displayed together.

jon-dez commented 4 years ago

And about the other commits, I honestly have no clue on how to create a pull request for a single commit.

natinusala commented 4 years ago

Yeah alright, that's fine with me.

To make a pull request for a single commit you need to push it to a separate branch and do the PR on that branch.

What I don't know is why do I see the changes in the diff, GitHub should have made empty commits out of the duplicate ones...

Whatever, can you clean the commits please (remove the duplicate ones)? I will merge then. Thanks!

jon-dez commented 4 years ago

It took a while to find out how to do it, but the duplicate commits are now removed.

natinusala commented 4 years ago

Thanks, I merged it