Skip to content

Mesh bugs#2424

Merged
willend merged 19 commits into
mainfrom
mesh_bugs
May 11, 2026
Merged

Mesh bugs#2424
willend merged 19 commits into
mainfrom
mesh_bugs

Conversation

@Lomholy
Copy link
Copy Markdown
Collaborator

@Lomholy Lomholy commented Apr 28, 2026

Free-form text area

Please describe what your PR is adding in terms of features or bugfixes:
Fixes the bugs mentioned in #2421 .

The errors in the mentioned bug were two fold. Firstly, when placing two boxes such that neithers vertices were within the other, the algorithm for checking what geometries overlaps with which failed.

The fix for this was to add an extra check in the overlap algorithm, to check if the edges of a mesh overlaps with another geometry. When this fix was implemented, the %Example value of Geometry_test dropped by 20 percent. This drop seems to be correct, and the test value is therefore changed.

Secondly, when placing a mesh in relation to one arm that is rotated, and placing the master in relation to another arm, the algorithm for checking overlaps would also fail. This was due to the rotation of shell points in the allocate_shell_points function being performed after moving the shell points to the current position, rather than before. This has been fixed.

A new test instrument that previously failed on both these accounts have been added. Thank you to Sam Lambrick for finding these bugs!

Furthermore this PR performs some Quality of life improvements to the Mesh code. Most notable among these is the switch from representing vertices as 9 double arrays, to 1 Coords array, and the addition of an error messages in case the mesh file cannot be opened.


Development OS / boundary conditions

Please describe what OS you developed and tested your additions on, and if any special dependencies are required:
MacOS Tahoe 26.4.1


PR Checklist for contributing to McStas/McXtrace

For a coherent and useful contribution to McStas/McXtrace, please fill in relevant parts of the checklist:

  • My contribution includes patches to an existing component file

    • I have used the mcdoc utility and rendered a reasonable documentation page for the component (please attach as screenshot in comments!)
    • I have ensured that basic use of the component is OK (e.g. an instrument using it compiles?)
    • I have used the mctest utility to test one or more instruments making use of the component (please attach mcviewtest report as screenshot in comments)
    • I have used the mccode-clangformat tool to apply the standard McCode component indentation scheme
    • I have used the mcrun --c-lint "linter" and followed advice to remove most / all warnings that are raised
  • My contribution includes a new instrument file

  • I have used the mcdoc utility and rendered a reasonable documentation page for the instrument (please attach as screenshot in comments!)
  • I have ensured that basic use of the instrument is OK (e.g. it compiles?)
  • ... and provided reasonable default parameters in that instrument that produce reasonable output
  • ... and maybe even added a %Example: line to describe expected behaviour
  • I have used the mcrun --c-lint "linter" and followed advice to remove most / all warnings that are raised
  • My new instrument is added within the examples hierarchy in a folder in the style of examples/ESS/New_stuff/New_stuff.instr
  • My new instrument has a new, unique filename, not clashing with existing example instruments
  • My new instrument requires a data/input file. If the datafile is specific for my instrument I have left it in the same example folder, but if general use I have placed it in the global data folder.
  • My PR is meant to fix a specific, existing issue

    • I have indicated the issue number here:
    • I have added documentation for the fix and possible side effects
  • My contribution contains something else

    • Explanation is added in free form text above or below the checklist

@Lomholy
Copy link
Copy Markdown
Collaborator Author

Lomholy commented May 7, 2026

Here a mcviewtest screenshot with the new Test instrument:

Image

And mcdoc for new Test instrument:

Image

@Lomholy
Copy link
Copy Markdown
Collaborator Author

Lomholy commented May 7, 2026

Union mesh mcdoc:
image

@Lomholy
Copy link
Copy Markdown
Collaborator Author

Lomholy commented May 7, 2026

@willend @mads-bertelsen This is almost ready for getting review. I only have one issue which seems to not be connected with this patch. Using the --c-linter on the new test instrument, I get these warnings:
image

@willend
Copy link
Copy Markdown
Contributor

willend commented May 7, 2026

@Lomholy, thanks for the update. There seems to be a merge-conflict in union-lib.c:
Screenshot 2026-05-07 at 13 50 56

@Lomholy
Copy link
Copy Markdown
Collaborator Author

Lomholy commented May 7, 2026

@Lomholy, thanks for the update. There seems to be a merge-conflict in union-lib.c:

You are very welcome.
The merge-conflict should be solved now.

@Lomholy Lomholy changed the title [DRAFT] Mesh bugs Mesh bugs May 7, 2026
Copy link
Copy Markdown
Contributor

@mads-bertelsen mads-bertelsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, the code quality has been improved a lot with this revision and both reported bugs are addressed! Only found a few print statements that was probably not intended for release?

Comment thread mcstas-comps/share/union-lib.c Outdated
Comment thread mcstas-comps/share/union-lib.c Outdated
Comment on lines +29 to +38
* %Example: Unused_par=1 Detector: psd_detector_I=10.7752
*
* %Parameters
*
* %End
********************************************************************************/

DEFINE INSTRUMENT Test_mesh_boxes(
Unused_par=0
)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed this, you don't need to have a parameter just for this? Example works if there is no parameters and nothing between '%Example:' and 'Detector'

Copy link
Copy Markdown
Collaborator Author

@Lomholy Lomholy May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was not aware. Didn't even test for having an example that didn't have a parameter. Just assumed it was necessary :D

I have now removed the parameter

@willend
Copy link
Copy Markdown
Contributor

willend commented May 11, 2026

At one point it was indeed required to have (even dummy) input parameters, but should no longer be the case.

@Lomholy
Copy link
Copy Markdown
Collaborator Author

Lomholy commented May 11, 2026

Alright, then with the removal of the parameter, I dare say that this PR is ready for you to push merge @willend

@willend
Copy link
Copy Markdown
Contributor

willend commented May 11, 2026

Ok @Lomholy, will do pending a quick look.

@mads-bertelsen
Copy link
Copy Markdown
Contributor

@willend @mads-bertelsen This is almost ready for getting review. I only have one issue which seems to not be connected with this patch. Using the --c-linter on the new test instrument, I get these warnings: image

This has actually been fixed in the logger transformation pull request so no need to fix it here too.

@willend
Copy link
Copy Markdown
Contributor

willend commented May 11, 2026

@Lomholy all good, ran a quick test on a GPU-box also - may require later (minor) intervention.

@willend willend merged commit 6721c09 into main May 11, 2026
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG]: Union_mesh fails on some intersection checks with other geometries, and rotation of shell points seem off

3 participants