New way to color tracks. #1579
New way to color tracks. #1579
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| void FairXMLDetectorColor::ColorizeNode(TGeoNode* node, FairXMLNode* xml, Int_t depth) const | ||
| { | ||
| TString name = xml->GetAttrib("name")->GetValue(); | ||
| TString node_name = node->GetName(); | ||
| Bool_t recursive = (xml->GetAttrib("recursive")->GetValue().Length() != 0 && !name.EqualTo(node_name)); | ||
| if (recursive && depth == 0) | ||
| return; | ||
| TString transparency = xml->GetAttrib("transparency")->GetValue(); | ||
| TString color = xml->GetAttrib("color")->GetValue(); | ||
| if (!color.EqualTo("")) { | ||
| node->GetVolume()->SetFillColor(FairXMLEveConf::StringToColor(color)); | ||
| node->GetVolume()->SetLineColor(FairXMLEveConf::StringToColor(color)); | ||
| } | ||
| if (!transparency.EqualTo("")) { | ||
| node->GetVolume()->SetTransparency((Char_t)(transparency.Atoi())); | ||
| } | ||
| if (xml->GetAttrib("recursive")->GetValue().Length() > 0) { | ||
| TString val = xml->GetAttrib("recursive")->GetValue(); | ||
| Int_t xml_depth = val.Atoi(); | ||
| if (recursive) { | ||
| xml_depth = depth - 1; | ||
| } | ||
| for (int i = 0; i < node->GetNdaughters(); i++) { | ||
| TGeoNode* daughter_node = node->GetDaughter(i); | ||
| ColorizeNode(daughter_node, xml, xml_depth); | ||
| } | ||
| } | ||
| if (xml->GetNChildren() > 0 && !recursive) { | ||
| for (int i = 0; i < node->GetNdaughters(); i++) { | ||
| TString subdetector_name = node->GetDaughter(i)->GetName(); | ||
| for (int j = 0; j < xml->GetNChildren(); j++) { | ||
| FairXMLNode* subnode = xml->GetChild(j); | ||
| TString subnode_name = subnode->GetAttrib("name")->GetValue(); | ||
| if (subnode_name == subdetector_name) { | ||
| ColorizeNode(node->GetDaughter(i), subnode, 0); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider refactoring for readability and performance.
The method ColorizeNode is functionally correct but can be refactored for better readability and performance. Specifically:
- The recursive logic can be simplified.
- The repeated calls to
xml->GetAttrib("recursive")->GetValue()can be optimized.
void FairXMLDetectorColor::ColorizeNode(TGeoNode* node, FairXMLNode* xml, Int_t depth) const
{
TString name = xml->GetAttrib("name")->GetValue();
TString node_name = node->GetName();
Bool_t recursive = (xml->GetAttrib("recursive")->GetValue().Length() != 0 && !name.EqualTo(node_name));
if (recursive && depth == 0)
return;
TString transparency = xml->GetAttrib("transparency")->GetValue();
TString color = xml->GetAttrib("color")->GetValue();
if (!color.EqualTo("")) {
node->GetVolume()->SetFillColor(FairXMLEveConf::StringToColor(color));
node->GetVolume()->SetLineColor(FairXMLEveConf::StringToColor(color));
}
if (!transparency.EqualTo("")) {
node->GetVolume()->SetTransparency((Char_t)(transparency.Atoi()));
}
if (recursive) {
Int_t xml_depth = xml->GetAttrib("recursive")->GetValue().Atoi();
if (depth > 0) {
xml_depth = depth - 1;
}
for (int i = 0; i < node->GetNdaughters(); i++) {
TGeoNode* daughter_node = node->GetDaughter(i);
ColorizeNode(daughter_node, xml, xml_depth);
}
} else if (xml->GetNChildren() > 0) {
for (int i = 0; i < node->GetNdaughters(); i++) {
TString subdetector_name = node->GetDaughter(i)->GetName();
for (int j = 0; j < xml->GetNChildren(); j++) {
FairXMLNode* subnode = xml->GetChild(j);
TString subnode_name = subnode->GetAttrib("name")->GetValue();
if (subnode_name == subdetector_name) {
ColorizeNode(node->GetDaughter(i), subnode, 0);
}
}
}
}
}
WalkthroughWalkthroughThe recent changes improve the event display framework's color management system by implementing a modular, XML-based approach for track and detector color assignments. Significant enhancements include the introduction of new classes for color handling, improved memory management practices, and a more flexible assignment mechanism that allows for dynamic configuration. This results in a more maintainable and user-friendly system for visualizing particle tracks and detector components. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FairEventManager
participant FairXMLPdgColor
participant FairEveMCTracks
User->>FairEventManager: SetXMLConfig("config.xml")
FairEventManager->>FairXMLPdgColor: Load configuration
FairEventManager->>FairEveMCTracks: Initialize
FairEveMCTracks->>FairXMLPdgColor: GetColor(pdgCode)
FairEveMCTracks->>User: DrawTrack(color)
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (2)
Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Please take a look at our Coding guidelines.
Especially G.3, G.4, and G.5.
I've added a few comments around. Those are just random.
| @@ -151,7 +156,6 @@ class FairEventManager : public TEveEventManager | |||
| TEveProjectionAxes* GetRPhiAxes() const { return fAxesPhi; }; | |||
| TEveProjectionAxes* GetRhoZAxes() const { return fAxesRho; }; | |||
| virtual void LoadXMLSettings(); | |||
| void LoadXMLDetector(TGeoNode* node, FairXMLNode* xml, Int_t depth = 0); | |||
There was a problem hiding this comment.
This is a public member function. So you're removing public API.
This is a breaking change.
If you really want this breaking change, then please write an appropriate CHANGELOG entry.
There was a problem hiding this comment.
I'm not sure if I made change corretly, please check :)
| void FairXMLDetectorColor::ColorizeNode(TGeoNode& node, const FairXMLNode& xml, Int_t depth) const | ||
| { | ||
| switch (depth) { | ||
| case 0: { // recursive mode of | ||
| return; | ||
| } break; | ||
| case -1: { // standard mode | ||
| Int_t newDepth = ApplyToNode(node, xml); | ||
| if (newDepth) { | ||
| for (int i = 0; i < node.GetNdaughters(); i++) { | ||
| ColorizeNode(*node.GetDaughter(i), xml, newDepth); | ||
| } | ||
| } else { | ||
| for (int i = 0; i < node.GetNdaughters(); i++) { | ||
| TString subdetector_name = node.GetDaughter(i)->GetName(); | ||
| for (int j = 0; j < xml.GetNChildren(); j++) { | ||
| FairXMLNode* subnode = xml.GetChild(j); | ||
| TString subnode_name = subnode->GetAttrib("name")->GetValue(); | ||
| if (subnode_name.EqualTo(subdetector_name)) { | ||
| ColorizeNode(*node.GetDaughter(i), *subnode, -1); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } break; | ||
| default: { // recusrive mode | ||
| ApplyToNode(node, xml); | ||
| for (int i = 0; i < node.GetNdaughters(); i++) { | ||
| ColorizeNode(*node.GetDaughter(i), xml, depth - 1); | ||
| } | ||
| } break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider refactoring the ColorizeNode method for readability and performance.
The method can be simplified by reducing redundant checks and improving the recursive logic.
void FairXMLDetectorColor::ColorizeNode(TGeoNode& node, const FairXMLNode& xml, Int_t depth) const
{
if (depth == 0) {
return;
}
Int_t newDepth = ApplyToNode(node, xml);
if (newDepth) {
for (int i = 0; i < node.GetNdaughters(); i++) {
ColorizeNode(*node.GetDaughter(i), xml, newDepth);
}
} else {
for (int i = 0; i < node.GetNdaughters(); i++) {
TString subdetector_name = node.GetDaughter(i)->GetName();
for (int j = 0; j < xml.GetNChildren(); j++) {
FairXMLNode* subnode = xml.GetChild(j);
TString subnode_name = subnode->GetAttrib("name")->GetValue();
if (subnode_name.EqualTo(subdetector_name)) {
ColorizeNode(*node.GetDaughter(i), *subnode, -1);
}
}
}
}
}| void FairXMLDetectorColor::Colorize(TGeoNode* node) | ||
| { | ||
| if (!node) | ||
| return; | ||
| ColorizeNode(*node, fNode, -1); | ||
| } |
There was a problem hiding this comment.
Add a check to ensure fNode is valid.
The method should include a check to ensure fNode is valid before calling ColorizeNode.
void FairXMLDetectorColor::Colorize(TGeoNode* node)
{
if (!node || !fNode)
return;
ColorizeNode(*node, fNode, -1);
}|
|
||
| ### Breaking Changes | ||
| * Event display | ||
| * Removed void FairEventManager::LoadXMLDetector(TGeoNode* node, FairXMLNode* xml, Int_t depth = 0) |
There was a problem hiding this comment.
Fix markdownlint issue.
Remove spaces inside emphasis markers.
- * Removed void FairEventManager::LoadXMLDetector(TGeoNode* node, FairXMLNode* xml, Int_t depth = 0)
+ * Removed void FairEventManager::LoadXMLDetector(TGeoNode* node, FairXMLNode* xml, Int_t depth = 0)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * Removed void FairEventManager::LoadXMLDetector(TGeoNode* node, FairXMLNode* xml, Int_t depth = 0) | |
| * Removed void FairEventManager::LoadXMLDetector(TGeoNode* node, FairXMLNode* xml, Int_t depth = 0) |
Tools
Markdownlint
11-11: null
Spaces inside emphasis markers(MD037, no-space-in-emphasis)
| auto colors = GetXMLConfigNode("MCTracksColors"); | ||
| auto detectors = GetXMLConfigNode("Detectors"); | ||
| if (colors) | ||
| fPDGColor = FairXMLPdgColor(colors); | ||
| if (detectors) { | ||
| auto cave = detectors->GetChild(0); | ||
| if (cave) { | ||
| FairXMLDetectorColor detCol(cave); | ||
| detCol.Colorize(gGeoManager->GetTopNode()); |
There was a problem hiding this comment.
Consider adding error handling in LoadXMLSettings.
The method should include error handling to manage cases where XML nodes are not found.
void FairEventManager::LoadXMLSettings()
{
auto colors = GetXMLConfigNode("MCTracksColors");
auto detectors = GetXMLConfigNode("Detectors");
if (colors) {
fPDGColor = FairXMLPdgColor(colors);
} else {
// Handle error
}
if (detectors) {
auto cave = detectors->GetChild(0);
if (cave) {
FairXMLDetectorColor detCol(cave);
detCol.Colorize(gGeoManager->GetTopNode());
} else {
// Handle error
}
} else {
// Handle error
}
gEve->Redraw3D();
}| @@ -84,7 +84,7 @@ void FairEventManager::Init(Int_t visopt, Int_t vislvl, Int_t maxvisnds) | |||
| fWorldSizeY = box->GetDY(); | |||
| fWorldSizeZ = box->GetDZ(); | |||
| } | |||
| if (!fXMLConfig.EqualTo("")) | |||
| if (fXMLFile) | |||
There was a problem hiding this comment.
Add a check to ensure fXMLFile is valid before calling LoadXMLSettings.
The method should include a check to ensure fXMLFile is valid before calling LoadXMLSettings.
if (fXMLFile && fXMLFile->IsValid()) {
LoadXMLSettings();
}
Currently the tracks coloring depens on node MCTrack colors, and the color scheme is configured by FairEventManager.
In this commit I propose the following changes:
Checklist: