search

JuliaPhysics / SolidStateDetectors.jl

Solid state detector field and charge drift simulation in Julia
Other
77 stars 29 forks source link

Refactor CSG #110

Closed lmh91 closed 3 years ago

lmh91 commented 3 years ago

I started refactoring our CSG. The core is finished. See branch refactorCSG.

This also concerns #62, #63, #84

The CSG is separated in a new submodule ConstructiveSolidGeometry inside SSD.

Rotation, translation and stretching/scaling are now implemented.

For now, I only added one primitive, the Tube.

I added a test script under /test/ConstructiveSolidGeometry/CSG_test.jl. At the moment is more an example script.

We can start implementing our other primitives and functionality, like read-in, to it or adept to it. But when adding something, please test for performance (type stability) like it is done in the test script. A PR to this branch must have tests.

Guideline for Primitives

General remarks

ToDo:

EDIT May 15th, 2021 The primitives have all been updated. Find a list of what still needs to be done at the end of this issue.

fhagemann commented 3 years ago

Repost from #141:

What still needs to be done:

lmh91 commented 3 years ago

Okay, I finally had some time looking into the code and have so the next days.

So far I have one remark:

Constructive Solid Geometry and volume primitives already implies that those primitives are not surfaces. So, a Box will never have no expansion in one dimension. If so, it should be turned into an surface primitive. This should happen in the construction and not anymore in functions handling those primitives, e.g. in get_decomposed_surfaces(b::Box). Also, functions like _is_torus_collapsed are also obsolete, because this "volume primitive" is actually than a "line primitive".

This also solves the problem with the type instability for the function get_decomposed_surfaces. This function should return a tuple of a fixed number (depending on the volume primitive) of surfaces primitives, which makes it type stable. E.g. get_decomposed_surfaces(b::Box) will always return 6 rectangles.

I will continue to look through everything.

lmh91 commented 3 years ago

Another point: :closed vs :open primitives. I think @fhagemann and I talked about this some time ago.

Its about whether the surface of a primitives belongs to the primitive or not. This is needed for differences in CSGs. It's a definition but we should agree on how to handle it. And in my opinion I would like to include the intersection surface of the two primitives in their difference: A - B -> A::Closed - Open(B) (I hope that syntax is clear here)

So all primitives would need an additional parametric type field. I would suggest to always use it as the second one as all primitives will have it. E.g. Sphere{T,TR} will change to Sphere{T,CO,TR} (CO for ClosedOpen but it does not matter). We can either use :closed or :open or add introduce types for it.

lmh91 commented 3 years ago

Now that all primitives have a origin and a rotation I would like to remove the from-to syntax for certain case from the configuration file format.

E.g. in case of the box primitives:

box:
  x:
    from: -1.0
    to: 1.0
  y:
    from: -2.0
    to: 2.0
  z:
    from: -3.0
    to: 3.0

I would suggest to not allow that syntax anymore because it influences the origin and instead go for something like:

box:
  widths: [2, 4, 6] # hX -> 2/2 = 1;  hY -> 4/2 = 2; hZ -> 6/2 = 3;
  origin: [0, 0, 0] # -> CartesianPoint{T}(0, 0, 0)

And also allow a possible rotation with the remaining rule that the rotation is performed before the translation.

With this I would also remove the z field which was a translation in z before. E.g. in the Torus example file:

torus:
  r_torus: 10.0
  r_tube:
      from: 2.0
      to: 2.0
  phi: 
      from: 0.0°
      to: 360.0°
  theta: 
      from: 0.0°
      to: 360.0°
  z: 0.0

For the angles or intervals like for r_in & r_out I would still keep the from-to syntax as, here, it does not influence the origin.

So in general I would go for:

primitive:
   <specific fields of the primitive>: ...
   origin: [x, y, z]
   rotation: <current rotation notation>

What do you think?

fhagemann commented 3 years ago

Why not allow for both? The from-to syntax can be easily "translated" to the current implementation of primitives?

lmh91 commented 3 years ago

Yes, the from-to syntax works right now also with the new version of the primitives.

I just find the definition with specific origin and rotation fields clearer as both might lead to confusions:

box:
  x:
    from: -1.0
    to: 1.0
  y:
    from: -2.0
    to: 2.0
  z:
    from: -3.0
    to: 3.0
  origin: [3, 3, 3]

Where should the origin now be? Sure, one could specify some rules how they are "ordered" but this might cause some confusion.

Prior to now we had just a z field for a translation in z. But now we have arbitrary translations so I would like to introduce the origin-field. Just adding x and y would also do but it might be unclear what it means and might conflict with other fields of certain primitives.

lmh91 commented 3 years ago

The cone can also be defined via z:

cone:
  r:
    bottom:
      from: 1.0
      to: 2.0
    top:
      from: 1.0
      to: 4.0
  phi:
    from: 0.0
    to: 6.283185307179586
  h: 2.0
  z: 
    from: 0
    to: 3

Here, z is ignored and h is used to define the cone from -1 to +1 in z.

If h does not exists z is used to define it from 0 to 3 in z.

lmh91 commented 3 years ago

I agree with your first point. I just named your translate inside the primitive definition origin.

But I think x, y and z should never represent any primitives. E.g., in case for the cone, z is bad description for the height (-> h is far better/clearer) of the primitive. And for the box just x, y and z are also a bit unclear as we also allow the syntax 

box:
  x: 1 # width in x: from -0.5 to +0.5 in x but the user might think it is -1 to 1
  y: 2
  z: 3

widhts or halfwidhts or hX, hY, hZ would be much clearer together in combination with origin. We can also add the syntax 

origin:
  z: 2

which would be identical to origin: [0,0,2].

Outside of the core primitive definition we still can wrap everything in translations and rotations via translate and rotate like we currently do. There I don't want to change anything.

fhagemann commented 3 years ago

Please feel free to edit this comment:

For merging v0.6 onto master

What still needs to be done for the v0.6 release:

Optional, e.g. v0.6.1

Optional (would go into v0.7)