Open fabien-colonnier opened 3 weeks ago
Thanks Fabien, plenty to think about on this. A couple of quick things first though: Make sure indentation is 4 spaces to match the rest of the code and see if you can ensure there is never whitespace at the end of lines.
The important thing with this one, as you've already said, is to get the naming conventions correct.
@optseb Thanks for your feedback. I'll try to correct for the indentations and whitespaces over the weekend or next week. I think the documentation in code style might need update as well
Naming suggestions, for the new items:
I've just finished some work that has been preoccupying me for the last week and finally had the headspace to take a look at this PR. There's lots to digest, but the first thing that comes to mind is whether we can define a base class for SimpleGridVisual
and ConstantAbscissaGraphVisual
so that these can be added to a morph::Visual
and so we don't have to pass a Visual
into the constructors for these objects. It would be nice to be able to do something like:
morph::Visual v(w, h, "Visual env");
auto gv = std::make_unique<morph::GraphVisual<blah>>(blah, blah);
auto gv_ptr = v.addVisualModel (gv);
auto colln = std::make_unique<morph::SimpleGridCollection<blah>>(blah, blah);
auto colln_ptrs = v.addVisualModel (colln); // or addVisualCollection (colln);
There are 2 things that needs to be accounted for in the case we inherit the Visual class.
bindmodel
function is used before and needs the reference to the visual objectclass Visual
might need to be amended, (not too sure how this could work actually)Also a point which I find interesting in the current approach is the ownership is transparent to the user. He doesn't have to understand that the first pointer is provided to the Visual object and a non-owning pointer is sent back. Then the update_graph
should be called from the same pointer of the object that has been created. There are maybe pros and cons to that but from a user perspective it gives some sort of abstraction to what's below.
I think we enter in the realm of what the user should know and what we think is not worth knowing, where it might be difficult to draw a clear line...
There are 2 things that needs to be accounted for in the case we inherit the Visual class.
I'm not thinking of inheriting the class morph::Visual. Not sure what you mean. I was thinking of having an interface class - something like morph::vmgroup::vmgroupbase
from which to derive a 'VisualModel group' which could be composed, and added to your morph::Visual
scene. If it can be made easy to make a new class from vmgroupbase by composing existing VisualModels, I think we provide a benefit to the user.
1. It is the fact that the `bindmodel` function is used before and needs the reference to the visual object 2. There are multiple objects to add so the default function in `class Visual` might need to be amended, (not too sure how this could work actually)
Also a point which I find interesting in the current approach is the ownership is transparent to the user. He doesn't have to understand that the first pointer is provided to the Visual object and a non-owning pointer is sent back. Then the
update_graph
should be called from the same pointer of the object that has been created. There are maybe pros and cons to that but from a user perspective it gives some sort of abstraction to what's below. I think we enter in the realm of what the user should know and what we think is not worth knowing, where it might be difficult to draw a clear line...
This is the essence of software architecture!
I've started doing some work on this on a local branch in your fork of the repo. I split out the "GridVisual plus ColourBar" class and made an example that just displays that. It's a starting point for getting the architecture right. We could have an in person meeting to discuss at some point.
https://github.com/ABRG-Models/morphologica/pull/276#issuecomment-2485286030 I didn't get what you meant initially. I think I do now. But definitely open for discussion to see how this can be implemented
Following on the issue https://github.com/ABRG-Models/morphologica/issues/167, I created the first aggregation of visuals into classes. I called them components for now. But all the naming should be reviewed to make sure it is scalable and understandable by the user.
I created a first example that show up what the output could be.
What is left to be done for this PR to be merged:
I am submitting this PR for now mainly to discuss naming and how we should arrange things in the repo