Split renderer

[nrxus]

This is my initial attempt at separating the LoadTexture (from the image feature) from the rest of the renderer. A lot of the changes are just rustfmt doing it's business as I coded away. It addresses #630

All tests pass, and all the examples run (the only example I could not run on my machine was the audio-queue-square-wave since it requires SDL2.0.4 and I have 2.0.0 but I did not change anything on the audio feature I am confident that it works.

Pros of my current approach:

• The SDL image context is now required for loading images, which means that we will not accidentally try to load an image after this context has been dropped.
• LoadImage is separate from the rendering functions which allows the user to create a ResourceManager that holds the ImageLoader (the sdl image context) without blocking the rendering functions.

Cons:

• Lifetimes went up. Both the Renderer and the Sdl2ImageContext hold a reference to the RenderContext which means that they now introduce a explicit lifetime.
• The users has to explicitely keep a hold of the RenderContext since neither the Renderer nor the Sdl2ImageContext own it. An alternative would be holding an Rc<RenderContext> which would get rid of the lifetime, and it won't force the user to hold the context just to keep it alive. A con to this would be having to store it in an Rc.

I tried to touch as little actual code as possible so the Renderer currently is still in charge of other texture creation methods. This could possibly also be moved but I wanted to get a WIP out first for review.

I would also like to get rid of the is_alive param that exists in the Texture and the RenderContext. This could possibly be done by using a lifetime similarly to how Font Loading works? The downside would be the added complexity that explicit lifetimes entail. From my experience Font Loading and caching were also a little hard to deal with. That being said it might be worth to standardize how we handle checking that the context is alive (lifetime vs the is_alive check).

I am open for any thoughts or nits to touch up on.

[Cobrand]

I will read this in further detail later but
... rustfmt doesn't make this very readable, I can't see what changed and what didn't at a quick glance. Is there a way to let rustfmt run on the master branch, commit that, and then apply your changes afterwards ? At least I should be able to see commits separately and discern what is truly a change and what is only rustfmt doing its job.

[nrxus]

I run rustfmt on the 12 files that I changed on master and did a rebase on the branch. The latest two commits should be rust-fmt changes free.

Also it seems like the build is only passing on nightly because of my use of crate-level protected fields. I quite like the use of them to prevent someone from the outside having access to those pointers but I can find ways around it if we are trying to aim at rust stable instead of only rust nightly.

[Cobrand]

You're right I was just looking at the PR diff as a whole and not separate commits (I didn't see them on mobile), my bad. I'll check it out as soon as I can.

[Xaeroxe]

pub(restricted) has been stabilized and as soon as we get a stable Rust 1.18 that syntax will be usable in stable.

[nrxus]

I got rid of pub(restricted) and field init shorthands to support stable 1.16 since that is the current version that Jenkins runs. It might be worth revisiting pub(restricted) when rustc 1.18 comes out.

[Cobrand]

(Looks like whitespaces changes on the other commit were not appreciated in this branch)

This isn't what I had in mind when you talked about that issue in #630. I'll tell you a little bit how I see things, feel free to correct me when I'm wrong and give your point of view when you disagree.

LoadImage is separate from the rendering functions which allows the user to create a ResourceManager that holds the ImageLoader (the sdl image context) without blocking the rendering functions

What do you mean by that ? Do you mean the possibility to load a texture and to display one in another thread ? Or do you mean just more flexibility in the code ?

Lifetimes went up. Both the Renderer and the Sdl2ImageContext hold a reference to the RenderContext which means that they now introduce a explicit lifetime.

Why does Sdl2ImageContext get a lifetime from RenderContext ? It doesn't make much sense, and it's quite limiting in some ways. If you have 2 renderers (say 1 Software and 1 OpenGL) and both use SDL2_image, both are fine until one of them is destroyed while the other is still active, where in that case Sdl2ImageContext.drop will call Image_Quit(), leaving with an error to one of them when it will try to load an image again. It also means that a Renderer in mandatory, while you can totally use SDL2_image without using a Renderer at all (in the C equivalent). In fact, SDL2_image internally loads its textures via SDL2_Surface first.

I would have imagined it like this: load_texture is still in the renderer or its equivalent, but &Sdl2ImageContext must be passed as a parameter of the method to ensure that Image is still alive when the call is made. That, or the other way: load_texture and load_surface are methods or Sdl2ImageContext, and load_texture takes a &Renderer as a parameter as well.

As for the separation between Renderer and RendererContext, I don't even see its use case. I never touched the Renderer itself so I didn't know it was there but I would have refactored everything into a single Renderer if it were me. I think the current API around Renderer is a total mess and very hard to use; I'm sure there is another safe way to do this properly. Unfortunately I'm having a hard time seeing what is the point of your current changes. If I'm mistaken in any way, please correct me.

[nrxus]

I meant flexibility in the code. As pointed in #630, the Renderer is currently in charge of both loading textures, and rendering textures. This makes it painful to implement a resource manager that loads the textures independently of the rendering functions.

The point of splitting the Renderer and the RenderContext is so that the RenderContext can be shared among separate code functionality that requires the context. In this case, both the rendering functionality and the loading functionality require the RenderContext (the raw pointer). The Renderer is now in charge of of the render methods, and still needs a &mut borrow for rendering. The ImageContext in the other hand needs it to get a Texture out of the loaded content.

I agree that Sdl2ImageContext should not hold a reference to the RenderContext. I like the idea of it having a load_surface and load_texture function, in which the load_texture needs a reference to the context. Alternatively, another struct could be created (i.e., TextureLoader) that holds a reference to both contexts and it is how we are able to load textures from image paths.

[Cobrand]

This is just an idea and I haven't tested this yet, but wouldnt an Rc<UnsafeCell<Renderer>> do the trick ? Rc almost costless compared to Boxes, the only cost you may have is when you heavily create/destroy references, but in your case you would have 1 reference for the display and another for the Texture loading, and nothing else for the whole execution beside that. It wouldn't be that far from a Box at that point. Tell me if that does the trick and if it doesn't, why.

The changes that you talked about in the other issue, I thought you mostly talked about how shitty it is to use the Renderer API. Notably the RenderTarget thing is really a pain to use right now, and I'm sure we could improve it.

[nrxus]

Having to use Rc<UnsafeCell> to implement something as common as a texture cache shows the difficulty of using the library. Also, currently I do not Box the renderer at all. If this was some edge-case that is particularly hard then maybe forcing the user to do some unsavory wrapping might be fine, but a ResourceManager is a very common structure, and something that should be easy to implement without having to rely on Rc<UnsafeCell<_>>. The way I envision this library as we reach 1.0 is that it provides a safe, usable, and intuitive interface around SDL2. Currently the API for the Renderer is still not there and this should be a good forward step.

I am open to changing this PR some, and fixing the SDL2ImageContext to not require the RenderContext as you said. The core of the story, though, is still valid.

There are also still some valid open questions. Should we move away from the renderer is_alive check within the Texture and move towards lifetimes in a similar way that the Font does it? Should we make every function in the image module a method of the SDL2ImageContext since they should not be called after this has been dropped?

[Cobrand]

Having to use Rc to implement something as common as a texture cache shows the difficulty of using the library

I agree that newcomers and even experienced users wouldn't think of that right away, but I don't think it's a bad idea either. While I think adding a lifetime to the textures are necessary, having lifetimes for the different instances of the references of Renderer is not and can (more or less easily) be bypassed with an Rc<UnsafeCell<_>>. There are in the current rust-sdl2 codebase Rc<>s and UnsafeCell<>s as well, but they are all hidden from the end user, so it's fine. As I said the cost of Rc<UnsafeCell<_>> is truly minimal. Even though you end up storing this on the heap instead of the stack, I feel like it's the most non-headache solution that I can find to share the Renderer between parts of the code that have almost nothing to do with each other.

I'm just thinking about this as an hypothesis but we can add another alias next to the current Renderer that would be an alias (an hidden one) for Rc<UnsafeCell<Renderer>>. We only would have to implement Deref so that all the methods of Renderer would be available for the alias. Things like Sdl2ImageContext.load_texture(&Renderer) could take a AsRef<Renderer> instead of a &Renderer, and we could implement AsRef<Renderer> for the said alias. That way if you truly want to not have any heap allocation, then go ahead and use the raw Renderer, and good luck with the lifetimes. Otherwise, we offer something that has a small overhead but that is both safe and easy to use. How does that sound ?

There are also still some valid open questions. Should we move away from the renderer is_alive check within the Texture and move towards lifetimes in a similar way that the Font does it?

I am inclined to remove is_alive and add lifetimes. This API is, as you said, bound to be safe and with a very thin wrapper. A wrapper as thin as possible without being unsafe would be ideal, those who do not want even the slightest overhead over their SDL2 calls are free to use sdl2-sys however they want. The lifetimes may add some complexity, but I feel like it's a necessary evil for a greater good in the end. Plus, the Textures have to be linked to the Renderer anyway, it doesn't make sense otherwise. So yeah, the lifetimes are definitely a must here.

Should we make every function in the image module a method of the SDL2ImageContext since they should not be called after this has been dropped?

Ideally, yes.

[nrxus]

Oh, did you mean having Rc<UnsafeCell<_>> within the Renderer in order to avoid having all those lifetimes? If so, I am 100% on board with that. I thought you meant that the user had to wrap the renderer we gave them with an Rc<UnsafeCell<_>>.

I was against the user being pushed to wrapping an interface that is supposed to already be safe and usable around Rc<UnsafeCell<_>> since we should be providing them an interface that does not require any extra wrapping for the general cases.

Should I make a separate PR to remove is_alive and replace it with lifetimes, or should I include it within this? It might be easier to do a separate PR since we both agree on that. Maybe also combine it with moving the functions on the image module to be methods of the context.

P.S. Should we rename SDL2ImageContext? It seems overly verbose. We already know we are within SDL2. I propose either ImageContext, or image::Context (it is already within the image module so it is implied that the context is for the image module)

[Cobrand]

At first I was talking about the user doing this by hand, but then I thought about the option that we give this as a more accessible alternative, while we still keep the option of a single heap-less Renderer in the API.

I think this middle ground would be a huge step forward: while Renderer would still have a kindof mediocre API, the changes (at least on Renderer only) wouldn't be breaking, but we would be offering newcomers a safe alternative.

[nrxus]

I think I updated my above comment right before as you were responding. I was wondering if I should make a separate PR for the changes that we both already agree on. (removing is_alive and moving the image function to be methods of Sdl2ImageContext) while we continue to tune this one which is a bit more complex.

Regarding the alias, it is not what I envisioned but I could implement it and see how it looks. Can you explain again the hesitation against the current separation of the Renderer and the RenderContext?

[Cobrand]

Yes, please do a separate PR.

I feel like having both Renderer and RendererContext is confusing, without even having used these two. By their names alone I can't tell what is their purposes or what is different between them. It's kind of the same with Window and WindowRef: I globally get the idea that one is only a reference and another is the real, owned thing, but that doesn't stop me from being confused every time. To my knowledge, this is the only crate that does something like this and this is very disturbing. I think they did this mainly for historical reasons, because Rust used to have way less features back then than it has now, and it led to some things being not-so-pretty in the codebase.

I'm sure we can find a way to refactor this in a really nice way that will leave no one confused, but I feel like having both Renderer and RendererContext is a step in the wrong direction.

[nrxus]

Aha! Naming is one of the hardest problems in Computer Science. I can definitely understand the confusion. I am by no means married to these names and I am open to any ideas.

I am also unsure what to call them exactly but I can explain the differences:

RenderContext is meant to be a struct that holds data. It is used explicitely for get actions on the SDL 2D rendering context. Such as, get_parent, get_window, get_surface, etc. It does not create or do anything. It is basically a bucket of data that signifies our current rendering context.

Renderer is meant to be a struct with a function. It is in charge of rendering using the borrowed RenderContext. To avoid complicating this PR even more I did not move the Texture Creating methods out of the Renderer but they should in theory also be extractable since they are not drawing methods.

Maybe we could rename it to RenderingContext vs Renderer. This might be more obvious that one is merely a context (holds information) while the other one is an actionable item (it renders). We might then have TextureCreator which also borrows from RenderingContext.

P.S. The new push was only to address the merge conflicts, there were no changes above.

[nrxus]

I was working on a separate PR to switch Texture's is_alive to lifetimes. Unfortunately, this is apparently impossible/really hard to do without first addressing the above change (in a way that the type alias would not be enough).

The way the lifetimes are set on the Font module, is that they are directly tied to the borrow on the Sdl2TtfContext. If we tried the same approach here, then the Texture would have a lifetime associated to a borrow of the Renderer. This would mean that now we cannot do any mutable borrows on the Renderer while this Texture is alive. This is unnaceptable since we will want to do a &mut call to copy to draw that Texture

I think this further highlights the fact that we need something separate from the Renderer that signifies whether the renderer context is currently alive or not.

[Cobrand]

I tried to do something as well on my side and it's definitely something that won't be easy to pull off. The fact that we require a "&mut self" every time for the Renderer is maybe not the right way of doing things ? If its methods were to take a &self instead, we could easily add lifetimes to the Texture so that it keeps track of Renderer. Taking &self instead of &mut self doesn't really matter in our case, since Renderer must be used on the same thread it has been created (It looks like it's a limitation of WinAPI).

Anyway, since Renderer cannot be used across threads, using immutable borrows instead of mutable ones would definitely solve the problem.

[Cobrand]

Here is some proof of concept: https://is.gd/q4ORzm. SharedRenderer have the same methods as Renderer via Deref, and you are free to use whichever you want based on your needs. I think this is the best solution, and it's not that ugly either.

For a TextureLoader/Manager, you simply need to clone one of the SharedRenderer you have in your TextureLoader struct, and then make the Texture from the shared renderer itself, and there you have your safe Texture that cannot be used after SharedRenderer is dropped ! How does it sound ?

Of course we will have to hack to unsafe code inside Renderer (because of the &self instead of the &mut self), and maybe use UnsafeCell sometimes as well, but since the unsafe isn't visible to the end user I think this is the right change to do.

[nrxus]

Making them all &self would solve the lifetime vs is_alive issue on the Texture as well as the creating of a ResourceManager issue. I assumed there was an underlying reason why we did a &mut self on the rendering calls which is why I did not suggest it.

You are correct that we cannot use the renderer but in the main thread, even for the surface -> texture calls.

In my view, having the renderering methods be &mut self make intuitive sense.
clear(&mut self): I am going to clear the window that my renderer holds,
copy(&mut self)I am going to copy pixels onto the window that my renderer holds.
Both of the above examples, imply change.

The Surface->Texture methods, in the other hand, do not seem to imply change but rather a usage of the context of my renderer to provide a Texture that can be drawn on the window. Granted this might be a small nitpick from my side since making them all &self does solve the issue at hand.

[Cobrand]

This won't be the first thread-safe struct to use &self instead of &mut self. Take for instance the TCP/UDP primitives in std. The linked method sets timeout to a certain value ? Surely it should take &mut self and not a &self ? I do agree with that, however since the underlying TCP implementation is thread-safe, &self can be used instead, and that's exactly what the std library do. If the standard library does it, I don't see any reason why we shouldn't.

On our side it is a little different: instead of being Thread-safe, Renderer cannot be used outside the main thread at all, hence it is inherently thread-safe. (by the way if someone could confirm if it either be on the main thread OR it should not be used in any other thread that the one he was initialized on, that's be great. If you find a reliable source, I'd love to read more about it.)

[nrxus]

I was looking this up earlier today and found this: https://forums.libsdl.org/viewtopic.php?p=48804 and http://www.glusoft.com/tuto/loading-screen-SDL2.php

They seem to imply that accessing the Renderer should only be done within the main thread. I could not find anything about it in any official documentation but I did not look hard enough.

Regarding this PR, should I rebase back to master and start a new? Seems like the best way to go now that we have a better plan of what we want to do.

To summarize:

• Make &mut self methods on the renderer be &self
• Remove is_alive from Texture (possibly also from the RenderTarget)
• Move functions that require the Sdl2ImageContext, methods of that context.

I believe that is all we discussed but let me know if I missed anything.

[nrxus]

After some digging, I found this:
https://wiki.libsdl.org/CategoryRender
https://bugzilla.libsdl.org/show_bug.cgi?id=1995

The explanation in the thread seems to imply the opposite of my previous understanding. In particular

When you create a context, you make it active on the thread you want to use it. You can only make it active on one thread at a time. If you don't do this, you'll crash in the way you saw.

Seems to imply that the renderer should only be used in the thread that it was created, albeit not necessarily in the main thread.

[nrxus]

I began a quick spike on this and found out that it will not be as easy as making all the methods &self.

There are some methods that truly want/need to be &mut self. In particular I am referring to: window_mut and surface_mut. This is because they return a &mut to an internal field.

This complicates things because we are now left with these possible choices:

1. Make these methods be &self and wrap the internal fields around a RefCell<>. This will allow us to return something that can be mutated within an immutable method. However, now a user could call this method twice and get two RefCell<_> pointed at the same data. This is unsafe at runtime if the user is not careful when they try to access the inner data.

2. Leave these methods as &mut self. This would mean that the moment a Texture is created (and we borrow the Renderer for the lifetime of the Texture), we cannot call on these two methods. It does not seem to me like the existence of a Texture should block the parent window/surface to the renderer to be modified. I could be missing something here and maybe it is expected that we cannot modify the underlying parent window/surface of the Renderer if a Texture currently exist for that Renderer. Let me know if I am mistaken.

[Cobrand]

There are some methods that truly want/need to be &mut self. In particular I am referring to: window_mut and surface_mut. This is because they return a &mut to an internal field.

This may seem a little extreme but after thinking about it I don't think the Renderer should own a reference over Window or Surface. I think it should be the other way around: Window can optionally hold an instance of Renderer. I don't like the fact that Renderer holds a reference to its parent, Rust implicitely encourages us to have composition over hard-to-read references and borrows, and I think this is exactly what we should do here.

I have imagined things like this :

struct Window {
window: *mut SDL_Window
}
struct Renderer {
renderer: *mut SDL_Renderer
}
impl Window {
}

// name is arguable
struct WindowWithRenderer {
    window: Window,
    renderer: Renderer
}
impl DerefMut for WindowWithRenderer {
type Target = Window;
}

impl Deref for WindowWithRenderer {
type Target = Renderer;
}

Now this is only an idea and I haven't tested its implications nor the how to do it directly. I guess there are tons of other problems linked with it, notably the fact that a Renderer can be on a Surface or on a Window, and that a Renderer can change its target to render into a custom Texture.

[nrxus]

I like the idea of the Renderer being held by the Surface or the Window. None of the the Renderer methods need them but for the get_parent methods. While the Renderer can be on a Surface or on a Window, after its creation it does not seem to care who his parent was as long as it stays alive.

I can play around with this and let you know how it looks.

[nrxus]

I paved over my old commits and implemented the suggestion. There are some slightly awkward stuff in there (-cough- RenderTarget -cough-) but overall I like this implementation.

There is something still in me that would like to separate the Rendering and the Texture Creating methods, but I can get over it if this feels nicer/simpler overall.

I named the new structs Canvas and SoftwareCanvas to differentiate a Renderer linked a Window vs a Renderer linked to a Surface. This imitates SDL_CreateRenderer vs SDL_CreateSoftwareRenderer. I would not be opposed to combining them and having an enum if you think that is more approachable.

The RenderTarget feels odd to me at this point, but it might not be worth handling it within this PR and maybe something to think about in the future. My main concern is that the RenderTarget does mutate the Renderer. However, since all of the Renderer methods now take in a &self, it hides away this mutation. For example, Object A holds a RenderTarget and Object B holds the Renderer. During some loop, Object A changes the Render Target to some Texture, then Object B does it's usual rendering job under the assumption that it is rendering as usual, when in fact it is now rendering to a Texture rather than to the Window. I feel similarly for some other methods that used to be &mut self but are now &self, but the RenderTarget is slightly more worrisome. The solution to this would be to split off the rendering methods (including creating a render target) from the texture creation methods. This way we can still have a lifetime on the Texture that prevents it from living past the SDL_Renderer while allowing the Renderer to have some a &mut self method for the RenderTarget. That being said, this is not a mountain I am willing to die on so if this is the simpler/nicer middle ground then it will work for my use-case.

[Cobrand]

No matter how I think about it if we want to implement RenderTarget we must give up the fact that Renderer can be shared at multiple locations and take &self for every method it has. I think you're right, we must make 2 separate structs, one which handles the resources of the Renderer, and another which handles the rendering directly. It's kind of weird to do it like that but I don't see any other safe, "rust" way of doing things in this case.

[Cobrand]

I think we're fine now. Would you mind squashing all of this into fewer commits ? You don't have to squash all of them into one, but at least separate rustfmt from everything else. And for !Send and !Sync, these are traits that prevent a future implementation of Send or Sync. This is useful for us since a Renderer should not leave the thread it was created in. However I'm not sure it's necessary, because as they use raw pointers I think they already implement !Sync and !Send.

…

On Apr 24, 2017, 20:57, at 20:57, nrxus ***@***.***> wrote: I reverted it back. Let me know if there is anything else that needs to be changed before merging. -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #632 (comment)

[nrxus]

I have squashed it all onto two commits (rustfmt + logical changes)

I could not find the documentation for !Send or !Sync, do you have a link where I can read about it?. is "!" a special syntax that means it cannot implement the trait (i.e., "!X" means it cannot implement X)?

The RendererContext has Rc<>'s in them which means they cannot be shared across threads. The Canvas holds an Rc<> to the RendererContext so it also cannot be shared across threads already so I think we are safe in that regard.

[Cobrand]

Thanks for the squash ! I will try to implement a very simple game (as another example for this repo) using your changes, to see if it's not that awkward to use. If you don't mind, I will push documentation and small fixes directly in your branch instead of asking you to make the change every time. I think it's overall all good now, but I can't say for sure that there aren't some changes to be made !

About !Sync and !Send, https://doc.rust-lang.org/nomicon/send-and-sync.html explains the basics.
Have a look at Sync as well, especially the "impl Sync for X" part. You'll see that Rc implements !Sync instead of Sync, that means that Rc cannot be shared between threads (you have to use Arc for that).

I think the "!X" syntax is only for Send and Sync, but I might be wrong. I've never seen it used anywhere else actually.

[nrxus]

That's cool. I learnt something today (:
I don't mind, feel free to do as you wish with the branch. I will make sure not to touch it for now.
I added two examples too so you can piggy off them.

[Cobrand]

At a first glance there is actually one huge change to be made in Canvas, in Drop, actually.

Since the order of the Drop is undefined as current rust 's standards, the Target can be dropped before the Renderer, which is obviously not good. The only option I see here is either using mem::forget and mem::drop manually, or using Option<_> to manually change Option<Renderer> so it's 100% sure that Renderer is dropped before Target.

I'm also somewhat uncomfortable with the creation process of TextureCreator; as it currently stands you have to hold both a Canvas and a TextureCreator to do anything remotely useful, and I'll try to see if there is any other alternative.

But don't worry, I'll take care of those changes, awesome work you made there, thanks again !

[nrxus]

It is totally OK to drop the target before dropping the RenderContext in the Canvas

The RenderContext holds an Rc<_> to the parent context (i.e., Window or Surface) so even though the target might drop, the SDL pointers won't be destroyed/free'd.

[Cobrand]

I see, thanks for the heads up I was just about to make useless changes !

[nrxus]

While it is true that you have to have both a Canvas and a TextureCreator to do anything useful with the Canvas, these do not have to be live in the same struct so I think it works fine. Object A is passed in a TextureManager so that it can load (hopefully cache'd) texures during creation. Object A also implements a draw method that requires a &mut Canvas. While it is true that both are needed, they are needed at very different stages of Object A and most objects don't need to keep a hold of them, only use them for a bit.

[Cobrand]

I managed to change Canvas to have one generic instead of two. I'd love to show them on your branch but looks like I have a permission denied, can you do something about this ?
Thanks !

EDIT: I think you have to tick "allow collaborators of this project to push to my branch" on this page or something like that.

[nrxus]

I have added you as a collaborator to my fork. Let me know if you can push now.

[Cobrand]

I pushed to your branch, tell me what you think about it. There are conflicts with master with the new Color struct, but from what I've seen this is very simple to solve, so either you can rebase on the new master or you can let me merge this the old school way into master.

I needed to add 2 traits to allow Canvas to have only 1 generic parameter, tell me what you think, I'm especially doubtful with the names.

[nrxus]

I did the rebase, run a rustfmt, and fixed a documentation test issue.

I was able to remove the TextureOwner trait since it was not providing any real value. Both possible contexts (WindowContext + SurfaceContext) implemented it, so it was not diffentiating anything between them. I like the solution you found of adding the context information to the texture target. I could have swore I tried doing it but I got stuck for some reason. Anyway, this looks good.

[Cobrand]

I think this looks great! Unless you have concerns, with your green light we can merge this. It would have been better with more documentation but we can still open another PR if we truly feel the need to do so.

Anyway, great job and thank you ! Nothing would have been possible without your tremendous help !

[nrxus]

It looks good to me, merge away! Thanks for your help and guidance on making this PR better than it was in the beginning!