For GDMLTube improvement

[lambdam94]

Hello Keith,

The SampleFiles/Problem_Files/tube.gdml and SampleFiles/Problem_Files/Tube1000.gdml files can be read if the createGeometry function in GDMLTube(GDMLcommon) is a little modified.
The following python code fixes the problem on my computer.

Regards,
Dam

begin createGeometry class GDMLTube(GDMLcommon) :

def createGeometry(self,fp):
# Need to add code to check values make a valid Tube
# need to take into account startphi != 0.0
# Define six vetices for the shape
mul = GDMLShared.getMult(fp.lunit)
rmax = mul * fp.rmax
rmin = mul * fp.rmin
z = fp.z * mul

   angleDeltaPhiDeg = 360.0
   if (hasattr(fp,'deltaphi')) :  
       angleDeltaPhiDeg = min([max([0.0, getAngleDeg(fp.aunit,fp.deltaphi)]), 360.0])

   cyl1 = Part.makeCylinder(rmax, z,
                            FreeCAD.Vector(0,0,0),
                            FreeCAD.Vector(0,0,1),
                            angleDeltaPhiDeg)
   if rmin > 0 :
      # Two cylinders
      cyl2 = Part.makeCylinder(rmin, z,
                            FreeCAD.Vector(0,0,0),
                            FreeCAD.Vector(0,0,1),
                            angleDeltaPhiDeg)
      fp.Shape = translate(cyl1.cut(cyl2),FreeCAD.Vector(0,0,-z/2))
   else :
      # Single Cylinder
      fp.Shape = translate(cyl1,FreeCAD.Vector(0,0,-z/2))

end createGeometry class GDMLTube(GDMLcommon) :

[KeithSloan]

Thanks but I much prefer to deal with startphi and deltaphi in the procedure angleSectionSolid rather than the individual graphic objects. Then if there is a bug it fixed for all. Also it should deal with startphi which your suggested code did not

I do not see/have and problem with the Tube files mentioned, I believe the issue with them
was fixed some commits ago. Please can you check you had the latest code when you experienced the problem.

[lambdam94]

Hello Keith Thanks for your answer  It is only a suggestion! I have done downloaded your new code this morning so I am sure that it is the last version. I have the same problem with my two laptops with ou without AppImage. Perhaps you can reproduce the pb with freecad.AppImage. I can 'try' all the gdml files of the deposit on my old laptops with freecad  and last gdml version and told you which one are ok or nok. Are you interesting? Regards Dam Envoyé depuis mon smartphone Samsung Galaxy. -------- Message d'origine --------De : Keith Sloan <notifications@github.com> Date : 23/04/2020 16:32 (GMT+01:00) À : KeithSloan/GDML <GDML@noreply.github.com> Cc : lambdam94 <lambdam94@gmail.com>, Author <author@noreply.github.com> Objet : Re: [KeithSloan/GDML] For GDMLTube improvement (#5) Thanks but I much prefer to deal with startphi and deltaphi in the procedure angleSectionSolid rather than the individual graphic objects. Then if there is a bug it fixed for all. Also it should deal with startphi which your suggested code did not I do not see/have and problem with the Tube files mentioned, I believe the issue with them was fixed some commits ago. Please can you check you had the latest code when you experienced the problem. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe. [ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "#5 (comment)", "url": "#5 (comment)", "name": "View Issue" }, "description": "View this Issue on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]

[KeithSloan]

Hi Dam

Not sure what is going on, the files load fine for me. Do you get any error/console messages.
What does your help FreeCAD version report.

Yes interested in what gdml files do not load.

Also one available at https://gitlab.cern.ch/VecGeom/VecGeom/tree/master/persistency/gdml/gdmls

[lambdam94]

Hi Keith,

the pb is for Freecad0.18.3 (the official ubuntu distribution)
I didn't see the pb under Freecad0.19.AppImage.
I "found" a new method to take into account startphi (you will find it hereafter). With this simple function, the oneTube.gdml works well under Freecad0.18.3 and under Freecad0.19.AppImage.

I will do other tests on CERN files,

Regards
Dam

def createGeometry(self,fp):
   mul = GDMLShared.getMult(fp.lunit)
   rmax = mul * fp.rmax
   rmin = mul * fp.rmin
   z = fp.z * mul

   if (hasattr(fp,'deltaphi')) :  
       angleDeltaPhiDeg = min([max([0.0, getAngleDeg(fp.aunit,fp.deltaphi)]), 360.0])
       cyl1 = Part.makeCylinder(rmax, z,
                        FreeCAD.Vector(0,0,0),
                        FreeCAD.Vector(0,0,1),
                        angleDeltaPhiDeg)
   else:
       cyl1 = Part.makeCylinder(rmax, z)   

   if rmin > 0 :
       # Two cylinders
      fp.Shape = translate(cyl1.cut(Part.makeCylinder(rmin, z)),FreeCAD.Vector(0,0,-z/2))
   else :
      # Single Cylinder
      fp.Shape = translate(cyl1,FreeCAD.Vector(0,0,-z/2))

   # if startphi -> rotate startphi angle
   if hasattr(fp, 'startphi') :
       newplace = FreeCAD.Placement(fp.Placement.Base,
                       FreeCAD.Rotation(FreeCAD.Vector(0,0,1), getAngleDeg(fp.aunit, fp.startphi)),
                       FreeCAD.Vector(0,0,0))        
       fp.Placement = newplace

[KeithSloan]

By the way are you running Windows or Linux?
Okay I just checked on FreeCAD 18.3 on my Linux system and see the error.

As to your proposed handling of startphi - I will have to think about as it has a problem in my view.

Objects Placement rotation gets exported as a rotation in the associated physical volume.
One can change things like startphi by changing/editing the GDML objects properties,
I don't think it is good to mix the two.
All the GDML objects have the parameters that can be changed via the property editor.
One can also change the rotation via the Placement in the property editor

[lambdam94]

I am running under (only) Linux.

I am agree with you.
Startphi, deltaphi, starttheta and deltatheta should not be mixed with the physical position and physical rotation.
During createGeometry, there are already some "translate" functions, which are hidden for the user. Some "hidden rotate" functions can be used during createGeometry (design of the shape), but as for translate functions they shouldn't be mix with physical placement and physical rotation (placement of the shape).

[KeithSloan]

I am agree with you.
Startphi, deltaphi, starttheta and deltatheta should not be mixed with the physical position and physical rotation.

So think rather than use Placement calls its easier to use

      fp.Shape = sphere.rotate(spos,sdir,startphi)

Which is what I tried with latest version of Sphere

[lambdam94]

Yes. A rotate without "placement" is better in createGeometry.
Do you think that you could do a commit?

[KeithSloan]

Okay, I altered angleSction so that it worked in my FreeCAD 0.18, give it a try

[lambdam94]

Hi Keith,

after an erase of GDML module and a new installation (to be sure that Addon give the last version),
the GDMLTube seems ok under freecad0.18 and under freecad0.19.AppImage

But, for the sphere,
under freecad 0.18.3, I have this message:
Traceback (most recent call last): File "/home/lambda/.FreeCAD/Mod/GDML/freecad/gdml/GDMLObjects.py", line 886, in execute self.createGeometry(fp) File "/home/lambda/.FreeCAD/Mod/GDML/freecad/gdml/GDMLObjects.py", line 939, in createGeometry fp.Shape = sphere.rotate(spos,sdir,startphi) <class 'TypeError'>: Property 'Shape': type must be 'Shape', not NoneType

under freecad0.19.AppImage, I have this result.

[KeithSloan]

Have not fixed the sphere yet. It is work in progress i.e. on a branch on my machine, Hope to make some progress this afternoon.

[lambdam94]

Hi Keith,

I have done another createGeometry for GDMLTube which needs low memory.
Could you take a look? Thanks

Regards
Dam

`
def createGeometry(self,fp):
# Need to add code to check values make a valid Tube
mul = GDMLShared.getMult(fp)
rmax = mul * fp.rmax
rmin = mul * fp.rmin
z = fp.z * mul

   if (hasattr(fp,'deltaphi')) :  
       angleDeltaPhiDeg = min([max([0.0, getAngleDeg(fp.aunit,fp.deltaphi)]), 360.0])
       tube = Part.makeCylinder(rmax, z,
                    FreeCAD.Vector(0,0,0),
                    FreeCAD.Vector(0,0,1),
                    angleDeltaPhiDeg)
       del angleDeltaPhiDeg
       if (hasattr(fp,'startphi')) :
           tube.rotate(FreeCAD.Vector(0,0,0),
                    FreeCAD.Vector(0,0,1),
                    getAngleDeg(fp.aunit, fp.startphi))
   else:
       tube = Part.makeCylinder(rmax, z) 


   if fp.rmin > 0 :
      tube = tube.cut(Part.makeCylinder(rmin, z))

   base = FreeCAD.Vector(0,0,-z/2)
   fp.Shape = translate(tube,base)

   # free memory -> del old var
   del base, mul, rmax, rmin, z`

[KeithSloan]

Wonder about the need to check hasattr as it is being done for some variables and not for others.
Given that all variables get given a value by the init. Think we should start to be consistent.
What is your view?
Will also check startphi for zero and in that case not rotate.

[lambdam94]

I think that variables given by the "outside" should be check.
But A check in every function is probably not necessary and time consuming.

Perharps we should directly refer to the GDML User's guide(lcgapp.cern.ch/project/simu/framework/GDML/doc/GDMLmanual.pdf)
i.e. for a Tube,

• 3 parameters (z, deltaphi and rmax) are always defined, "check hasattr" are always true
• 2 parameters (rmin, startphi ) are option, "check hasattr": good option

Yes, we are close to be consistent!

[KeithSloan]

But what the manual refers to is the variables in the GDML file.
Once parsed GDMLTube is called with the parameters, if one does not always specify the optional ones like rmin and startpi in the constructor then those values would not be available to change via the property editor.

If all parameters are specified on the init as is the case now, then there is no need to check with hasattr

[KeithSloan]

Freeing memory. Why is this necessary.

Quote from the web

Python is garbage-collected, which means that there are no guarantees that an object is actually removed from memory when you do 'del someBigObject'. In fact, doing 'del someObject' is not only pointless, it's considered bad style.

[KeithSloan]

Please try latest commit

[lambdam94]

That seems good too!
Thanks Keith!

The "real" test will be the impact on large file like 'lhcbvelo.gdml'.
About free memory, my teachers told me that for a list you should delete all the object not only the name of the list (del liste[:]). In fact I try to verify this. Perhaps we can try to verify this in specify conditions.
Perhaps we could do an huge number of createGeometry ( for indice in range(100000): createGeometry) and look at the time computing and memory .

[KeithSloan]

Are you sure your teacher is referring to Python?

It is something that should be done for c++, but for Python I don't think del frees any memory, just disassociated the variable.

https://docs.python.org/2/reference/simple_stmts.html#del