Ensure proper colorer range for depth range #172
Conversation
|
Will you please rebase the PR? |
graph/assemblygraph.cpp
Outdated
| @@ -51,6 +51,7 @@ | |||
|
|
|||
| AssemblyGraph::AssemblyGraph() | |||
| : m_sequencesLoadedFromFasta(NOT_READY) | |||
| , m_currentScope(graph::Scope::wholeGraph()) | |||
There was a problem hiding this comment.
I do not think this is a proper approach. AssemblyGraph is a data structure and it should not "know" about the drawing scope, so this way you're leaking the abstraction through unrelated data structure.
Certainly, we do have some legacy issues (e.g. global settings state that is changed in different places, but we should improve the situation, and not make it worse).
graph/nodecolorer.cpp
Outdated
| @@ -76,8 +76,13 @@ QColor DepthNodeColorer::get(const GraphicsItemNode *node) { | |||
|
|
|||
| double lowValue = g_settings->lowDepthValue, highValue = g_settings->highDepthValue; | |||
| if (g_settings->autoDepthValue) { | |||
| lowValue = m_graph->m_firstQuartileDepth; | |||
| highValue = m_graph->m_thirdQuartileDepth; | |||
| if (m_graph->currentScope().graphScope() == DEPTH_RANGE) { | |||
There was a problem hiding this comment.
So, instead of fetching the scope from the graph (which is not really something belongs to it), it would be better to route it through DepthNodeColorer, or rather calculate low / high value when colorer is created.
When graph is redrawn, scope is passed to colorer which calculates low and high values and stores then for colorers to use when needed. Is this correct way to store them? resolves asl#161
ui/mainwindow.cpp
Outdated
| @@ -1025,6 +1025,7 @@ void MainWindow::drawGraph() { | |||
| resetScene(); | |||
| g_assemblyGraph->resetNodes(); | |||
| g_assemblyGraph->markNodesToDraw(scope, startingNodes); | |||
| g_settings->nodeColorer->setGlobalScope(scope,g_assemblyGraph->m_firstQuartileDepth, g_assemblyGraph->m_thirdQuartileDepth); | |||
There was a problem hiding this comment.
This does not look right. Colorers are transient, they are recreated as necessary, so, on colorer switch this will be lost.
graph/nodecolorer.h
Outdated
| @@ -51,7 +53,15 @@ class INodeColorer { | |||
| static std::unique_ptr<INodeColorer> create(NodeColorScheme scheme); | |||
|
|
|||
| [[nodiscard]] NodeColorScheme scheme() const { return m_scheme; } | |||
|
|
|||
| static void setGlobalScope(const graph::Scope &scope, double firstQuartileDepth, double thirdQuartileDepth); | |||
There was a problem hiding this comment.
Why this is a part of common interface? It only belongs to depth colorer.
|
I am not sure I understand how to do it best. Colorers are transient, so that means we need to store either the scope or the calculated lowValue and highValue somewhere so they can be acessed by the depth colorer, correct? |
|
The colorer is switched in a single place: https://github.com/asl/BandageNG/blob/dev/ui/mainwindow.cpp#L1364 (while it is currently in settings, it just it was not untangled fully from the global state, it really should belong to mainwindow, or rather we'd need to have So, you might want to check if the current scheme is coloring by depth, then cast interface to instance of DepthColorer and pass the scope there. The DepthColorer would recalculate min / max values then. |
Store Scope reference when graph is redrawn. Scope reference is stored only in a private static variable of DepthNodeColorer, which persists when colorers are changed. As DepthNodeColorer instance might not exist when scope is changed, reference is saved by a function defined in INodeColorer. resolves asl#161
fixes asl#161 build errors
This reverts commit 116882a.
Ubuntu >24.04 image does not longer include imagemagick by default. fixes asl#161 Linux tests fails
|
Is my latest approach in 91f7cf2 adequate? |
resolves #161