Visualization problem

[MilanKlausz]

https://github.com/mctools/ncrystal/blob/6dd1504ca521ce7f717f5f455efb07afea5329e2/ncrystal_mcstas/NCrystal_sample.comp#L327 According to the second comment in the implementation of the _mcdisrectangle function:

void mcdis_rectangle(char* plane, double x, double y, double z,
             double width, double height){
  /* draws a rectangle in the plane           */
  /* x is ALWAYS width and y is ALWAYS height */

the linked line should be replaced with mcdis_rectangle("yz", 0., 0., 0., geoparams.dz,geoparams.dy);` to get the projections of a box.

Or the whole NC_BOX case could be solved with one line: mcdis_box(0., 0., 0., geoparams.dx, geoparams.dy, geoparams.dz); It looks better in my opinion but there might be a reason why it wasn't used in the first place.

[tkittel]

Thanks a lot for another very useful report @MilanKlausz !

Indeed, I can confirm that using your mcdis_box line gives the visualisation that was actually intended, so I will make a new patch release with that.

However, I would like to take this opportunity to double-check all of our visualisation code and perhaps improve the sphere/cylinder representations as well. Perhaps @willend or @ebknudsen can give a few pointers on how they recommend visualising cylinders and spheres? Currently we cludge together a few circles and lines, but in my past life working on a viewer, we left the triangulation of a curved surface up to the viewer (since it depended on a tradeoff between "too many lines and I can't see stuff inside the shape" and "too few lines and I can't actually tell where the curved shape is"). But I don't find any mcdis_sphere or mcdis_cylinder functions in mccode-r.h, so the question for Peter and Erik is whether or not there are some recommended MCDIPLAY keywords we should output to get spheres/cylinders that look nice. Or we could add a few more "longitudinal lines" in the sphere visualisation.

Or is the code we have now already in line with how it is recommended by mcstas gurus?:

MCDISPLAY
%{
  mcdis_magnify("xyz");
  switch (geoparams.shape) {
  case NC_CYLINDER:
    mcdis_circle("xz", 0,  geoparams.dy/2.0, 0, geoparams.radius);
    mcdis_circle("xz", 0, -geoparams.dy/2.0, 0, geoparams.radius);
    mcdis_line(-radius, -geoparams.dy/2.0, 0, -geoparams.radius, +yheight/2.0, 0);
    mcdis_line(+radius, -geoparams.dy/2.0, 0, +geoparams.radius, +yheight/2.0, 0);
    mcdis_line(0, -geoparams.dy/2.0, -geoparams.radius, 0, +geoparams.dy/2.0, -geoparams.radius);
    mcdis_line(0, -geoparams.dy/2.0, +geoparams.radius, 0, +geoparams.dy/2.0, +geoparams.radius);
    break;
  case NC_BOX:
    mcdis_box(0., 0., 0., geoparams.dx, geoparams.dy, geoparams.dz);
    break;
  case NC_SPHERE:
    mcdis_circle("xy", 0., 0., 0., geoparams.radius);
    mcdis_circle("xz", 0., 0., 0., geoparams.radius);
    mcdis_circle("yz", 0., 0., 0., geoparams.radius);
    break;
  };
%}

Finally, is it correct that we have placed a call to mcdis_magnify("xyz");?

[willend]

Hi @tkittel,

It is correct that we (currently) have no mcdis_cylinder or mcdis_sphere macros, but the idea is of course meaningful. :-) (@ebknudsen has assigned himself a ticket to implement something like this: https://github.com/McStasMcXtrace/McCode/issues/595) But for now, the standard solution is to do exactly what you are doing - "stitching" together something that looks roughly right from the line macro, 90'ies style...

It is on our list to make a more "modern" set of visualisation macros, but lots of other more important stuff has a higher priority. :-)

Wrt. mcdis_magnify I believe it was originally meant for selecting between 3 set of axis-parallel planes, i.e. xy, xz, yz - but my feeling is that it is currently in practice unused. I believe you can leave it out alltogether, but let me check and get back to you.

[tkittel]

Ok, thanks a lot for the quick feedback @willend. I'll look out for the future resolution of McStasMcXtrace/McCode#595, but until then just keep the current code. I agree this is nice-to-have and not super critical.

So just waiting to hear back about mcdis_magnify, before I nuke that line as well :-)

[willend]

Hi there,

I've investigated, and what mcdis_magnify does is really very little - and the functionality is a little weird:

• Only when using our soon-to-be-completely-deprecated Perl-PGPLOT visualizer and giving a --zoom=value, each component would itself decide "on which plane" the that zoom factor would be applied when drawing the component.

• If not used, the component would simply "not zoom"

• Both the tool in question and the other tools have interactive zoom implemented

• Hence I have simply deprecated this (not very well-known) functionality from the tools and mccode-r.c, see https://github.com/McStasMcXtrace/McCode/commit/eb6902645eb77cad74b4645c6cd6d58456d6e206 and https://github.com/McStasMcXtrace/McCode/commit/96a51b19e773487220c3c3dba263987e273bb517

Meanwhile @ebknudsen is making good progress on _cylinder and _sphere calls - that will of course not be available for the users until McStas 2.5 ;-)

[tkittel]

Ok, I feel less stupid then for not really understanding mcdis_magnify :-)

I'll remove it then - and watch McStasMcXtrace/McCode#595 for a future migration to use the new glorious sphere/cylinder calls :-)

[tkittel]

NCrystal v0.9.6 is now released with the box-visualisation fix. Closing.