-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding support for IrregularCaloCells physics objects #536
Conversation
tagging @etiennedreyer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the PR! I am sure this new physics object works as intended but I added a few comments for some Phoenix specific things that may not have been clear.
packages/phoenix-event-display/src/loaders/objects/phoenix-objects.ts
Outdated
Show resolved
Hide resolved
packages/phoenix-event-display/src/loaders/objects/phoenix-objects.ts
Outdated
Show resolved
Hide resolved
packages/phoenix-event-display/src/loaders/objects/phoenix-objects.ts
Outdated
Show resolved
Hide resolved
Another comment - it seems like the linting check failed:
If you're using Visual Studio Code, you can add the prettier plugin and then 'Format Document' with this linting tool. Edit: I should have also linked to the documentation: |
Hi @9inpachi thanks for reviewing and suggesting the changes. We implemented them. |
No objections from my side, but I'll let @9inpachi comment too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me too. Thanks for making the changes. :)
cell.name = 'IrregularCaloCell'; | ||
|
||
// Setting uuid for selection from collections info | ||
irrCells.uuid = cell.uuid; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no uuid in irrCells ! So my ci (LHCb's deployment one) gives the following error :
src/loaders/objects/phoenix-objects.ts(834,14): error TS2339: Property 'uuid' does not exist on type '{ type: any; layer: number; vtx: any; color: string; opacity: any; }'.
I do not get why we did not see it in the official repo ! Can you fix it ?
layer: number; | ||
vtx: any; | ||
color: string; | ||
opacity: any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @sponce we looked together and are not fully sure how to address this. We checked that the browser runs for us (with and without IrregularCaloCells) before pushing, but somehow it's not working the LHCb ci.
Should we just add the following line here?
uuid: string;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the meantime, we've identified what might be the reason for different behavior in LHCb : we somehow use threejs 148 rather than 147. So I suppose moving back to 147 would make it work for us.
On the other hand, the code in indeed weird, as you set a member that does not exist. So indeed, I would add uuid:string
or alternatively drop the line setting the uuid if it's not used. But I suppose it is in your case (for lhcb, I did that stop gap fix yesterday and it worked)
Calorimeter cells with arbitrary polyhedron-type geometry.
Each cell is represented by 8 (x,y,z) vertices connected using a convex hull from the ConvexGeometry class.
IrregularCaloCells is a list of objects with the following attributes :
layer
- the Calorimeter layervtx
- a flattened list of 8 vertex coordinates (24 floats)color
- an integer list of [R,G,B] valuesopacity
- value from 0 to 1