search

KeithSloan / GDML

FreeCAD GDML Workbench - AddonManager Installable
Other
50 stars 17 forks source link

For GDMLTube improvement #5

Closed lambdam94 closed 4 years ago

lambdam94 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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": "https://github.com/KeithSloan/GDML/issues/5#issuecomment-618429270", "url": "https://github.com/KeithSloan/GDML/issues/5#issuecomment-618429270", "name": "View Issue" }, "description": "View this Issue on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]

KeithSloan commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

KeithSloan commented 4 years ago

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

lambdam94 commented 4 years ago

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. sphere-pb

KeithSloan commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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,

Yes, we are close to be consistent!

KeithSloan commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

Please try latest commit

lambdam94 commented 4 years ago

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 commented 4 years ago

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