chbergmann / OpticsWorkbench

GNU Lesser General Public License v3.0
69 stars 23 forks source link

Diffraction grating support #36

Closed MSLDgit closed 1 year ago

MSLDgit commented 1 year ago

I changed the code, so that it is on pair with the newest version of OpticsWorkbench. Additions are the support for simple diffraction gratings and the order parameter of rays, needed to define the order of diffraction. I updated the glass material catalogue with some new materials. The getMaterial function is used by the grating class aswell, so it is now as a standalone function. A bug was removed happening during total reflection wich would assign the index of refraction of the 2nd material to the last indices of refraction (if total reflection on lens-air surface this would be 1, which is ok, but if on lens-lens surface this would be n of the second lens, which would mess up the "last index of refraction chain"). As the last index of refaction does not change during total reflection, nothing new should be applied to this list, i think. There is now an Icon for diffraction gratings. Pls let me know problems, or if I did do nonsense during this git-associated process (still trying to handle that all).

chbergmann commented 1 year ago

Thank you for committing. There are only a few things for you to do before I can merge:

When you are ready, I will squash merge your branch, this means all commits will be combined into a single one.

MSLDgit commented 1 year ago

Hi, I ran the 2d example and the lens objects are initiated with refraction idex of 1. Is this what you are referring to with the total reflection? Set an index to one of the materials or via typing in a number or sellmeier coeficients, and you will see it should work fine..

MSLDgit commented 1 year ago

The initiation with the refactive index of 1 is due to a change of the getMaterials function, where None values and negative values in the sellmeier coeficients lead to the result, that one is not able anymore to cycle through the material list, so I changed that to a default of 1 when a ? material is selected, instead of None

chbergmann commented 1 year ago

I see, your bugfix was correct, but the example was broken. Please change line 93 in examples/example1.py to OpticsWorkbench.makeLens([Sketch_Lens, Sketch_Prism], material='NBK7/Window glass')

chbergmann commented 1 year ago

and please remove the printfs you added for debugging

MSLDgit commented 1 year ago

Yes will do, I don't know if I am able to do this today or just can finish tomorrow. I hope thats ok.

chbergmann commented 1 year ago

no problem

MSLDgit commented 1 year ago

Hi, I am wondering about Sunray.py. It is, if I don't confuse it somehow, still used, also in the original branch, where inOpticsWorkbench.py in line 58 and later in line 71 to 75 it is used to generate the Sunray object with even its own icon in the Combo View-Model Viewer. One could leave this whole sunray.py and these lines out, and instead use an older version of the sunray. This version is still present in the branch I was working with, and you can findit commented out in lines 88-109 in the OpticsWorkbench.py file of the branch I did a pull request with. This would, however, not generate a sunray object, but a freecad group called sunray, containing the rays generated for the sunray. Maybe I'm missing something here, but I think this would be your decision, whether to leave it out, or not. The differences I see, is the seperate Icon in the model viewer for the sunray, which is not present for the group. The ability to move the sunray object itself, and also the contained rays, which is not possible with a group (here you would have to select all rays in the sunray group and move them in a single translation...). However, e.g., the sunray object is easy to delete accidentially, for groups, freecad asks you if you want to delete a group and all objects it contains.

Edited for some typos...

MSLDgit commented 1 year ago

another thing would be of course to move the content of the sunray.py file to the OpticalObjects.py file... Edited for some typos

MSLDgit commented 1 year ago

The initiation with the refactive index of 1 is due to a change of the getMaterials function, where None values and negative values in the sellmeier coeficients lead to the result, that one is not able anymore to cycle through the material list, so I changed that to a default of 1 when a ? material is selected, instead of None

actually I think this would solve issue #12

MSLDgit commented 1 year ago

Chbergman, please note that I am working on a branch, finalizing the pull request, according to your requirements. The last thing would have been a decision regarding the sunray.py file. After sorting that out a pull request with a clean (free of prints and clean readme) branch would have followed.

chbergmann commented 1 year ago

I am sorry that I interrupted your work and merged to branch. I was thinking it is easier if I do the final corrections (only a few) myself instead of having long discussions.

MSLDgit commented 1 year ago

Ja, I noticed that^^. Well, If you are happy with the output, it's fine for me. Another quick annotation and then I am done. I used thorlabs step files, which are free to download, for the example screenshots. While it shows that you can do this, which is very nice, I don't know about the licence conditions for these files. This is also not stated on their website. Maybe it is not a problem, but I thus, personally would have left out the example images with thorlabs step-files for this workbench readme, as your workbench does have some popularity, to not run into licence problems with thorlabs. The example imageas are the ones with the concave mirror grating and, somewhat less noticable, the echelle spectrometer. Best Miles

chbergmann commented 1 year ago

Don't worry. We did not upload any step files. Having only pictures of them in some screenshots should not be a problem.