Skip to content

Conversation

@Lomholy
Copy link
Collaborator

@Lomholy Lomholy commented Jan 25, 2026

Free-form text area

Please describe what your PR is adding in terms of features or bugfixes:
This PR fixes two bugs. Firstly it fixes a bug in merge_lines_to_draw that would exit if no lines were to be drawn. This happened during occasions where the entirety of the shape (or subparts of it in the case of the cylinder tops) where completely enveloped in another shape. Such as (the silly) example of one cylinder with a priority of 1, inside a bigger cylinder with a priority of 10.

Secondly this PR adds a reallocation to the dummyint when mesh is the input shape, to draw_line_with_highest_priority. This would fail originally overflow when more than 2 intersections where the case for a line.


Development OS / boundary conditions

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


PR Checklist for contributing to McStas/McXtrace

  • My contribution contains something else

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

@willend
Copy link
Contributor

willend commented Jan 25, 2026

These edits look OK meaningful. Could you maybe include an example instrument with the mentioned (silly) geometry corner-case? (So that it can easily be demonstrated that "things now work”.)

@Lomholy
Copy link
Collaborator Author

Lomholy commented Jan 26, 2026

Hi Peter,

Yes of course. Said example instrument should be attached here:

testing.txt

I also have an instrument where the mesh would crash before, but doesn't with the current changes, but it is too voluminous to attach here.

@willend
Copy link
Contributor

willend commented Jan 26, 2026

Thanks for that. I’d prefer to have it placed as part of the PR, e.g. in Tests_union/Hidden_cylinder/Hidden_cylinder.instr (renamed from the generic ‘imaging’)

(And with updated documentation header so that others have a chance to understand what it does ;-) )

@Lomholy
Copy link
Collaborator Author

Lomholy commented Jan 26, 2026

Ah of course. Then I will also add the mesh test there.

@willend
Copy link
Contributor

willend commented Jan 26, 2026

If the two tests come in different instrument files, please add them with different names / folders in the hierarchy :-)

@Lomholy
Copy link
Collaborator Author

Lomholy commented Jan 26, 2026

Of course!
The many meshes test requires some quite large mesh files (Total folder size ~76 MB). Should I skip it?

Regarding the example, how can I force it to test for display? Something like:

  • %Example: -t 1 Detector: det_I=3.42884e-06
    ?

@willend
Copy link
Contributor

willend commented Jan 26, 2026

Hmm yes I think 76 megs of STL is a bit much. :-) I would prefer a bare minimum to show whatever we want to show. :-)

And sorry there is no way of running mcdisplay in testing at the moment - but I think it would be fine to run a corresponding simulation (which is what the %Example line would do) and maybe add an instruction in writing above mentioning what to do for the related mcdisplay run?

@Lomholy
Copy link
Collaborator Author

Lomholy commented Jan 26, 2026

Hi again, I was able to cut it down to 1 stl file instead of the many I had before. It should now fail on main, and work on this branch with the mcdisplay.

@willend
Copy link
Contributor

willend commented Jan 26, 2026

Confirmed, mcdisplay runs based on. current main fail with

raceback (most recent call last):
  File "/Users/peterwillendrup/micromamba/envs/mcstas-dev/share/mcstas/tools/Python/mcdisplay/pyqtgraph/mcdisplay.py", line 579, in <module>
    main(**args)
    ~~~~^^^^^^^^
  File "/Users/peterwillendrup/micromamba/envs/mcstas-dev/share/mcstas/tools/Python/mcdisplay/pyqtgraph/mcdisplay.py", line 536, in main
    instrument = reader.read_instrument()
  File "/Users/peterwillendrup/micromamba/envs/mcstas-dev/share/mcstas/tools/Python/mccodelib/mcdisplayutils.py", line 46, in read_instrument
    self.pipeman.join()
    ~~~~~~~~~~~~~~~~~^^
  File "/Users/peterwillendrup/micromamba/envs/mcstas-dev/share/mcstas/tools/Python/mccodelib/pipetools.py", line 345, in join
    raise self.reader.exc_obj
  File "/Users/peterwillendrup/micromamba/envs/mcstas-dev/share/mcstas/tools/Python/mccodelib/pipetools.py", line 284, in run
    raise Exception("TraceReader - mcrun process exited with code: %s" % str(poll))
Exception: TraceReader - mcrun process exited with code: 1

whereas the PR codebase provides functional visualisation of the two instruments:
Screenshot 2026-01-26 at 10 52 45

Screenshot 2026-01-26 at 10 53 08

Will merge once the tests have made it through.

@willend
Copy link
Contributor

willend commented Jan 26, 2026

After a couple of manual re-runs (temporary failures deploying to Windows systems) all seems OK.

Also, I have rectified your instr headers for matching Instrument: labels and added short description strings (all described under https://github.com/mccode-dev/McCode/wiki/Component-development - work in progress component-developer docs)

@willend willend merged commit 453f8ff into main Jan 26, 2026
36 checks passed
@Lomholy Lomholy deleted the union_mcdisplay_bug branch January 26, 2026 12:20
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.

3 participants