bugGNU Octave - Bugs: bug #48109, Trisurf fails to colour triangles...

 
 

bug #48109: Trisurf fails to colour triangles correctly.

Submitter:  None
Submitted:  Fri 03 Jun 2016 09:03:02 AM UTC
   
 
Category:  Plotting Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  Fixed Assigned to:  None
Originator Name:  Tim D Originator Email:  -email is unavailable-
Open/Closed:  * Closed Release:  * 4.2.0
Operating System:  * Any Fixed Release:  None
Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Mon 27 Nov 2017 05:33:58 AM UTC, comment #10: 

Turns out the same input validation was being used for trimesh as well.  I used your template from trisurf to also update trimesh and checked in both changes under your name here http://hg.savannah.gnu.org/hgweb/octave/rev/678912855789.

Since you are now officially a contributor to Octave, would you like to have your name appear at the beginning of the manual?  It's up to you.  Some people like it, others prefer to remain more anonymous.

Marking bug as fixed and closing report.

Rik <rik5>
Group administrator
Mon 20 Nov 2017 09:25:24 PM UTC, comment #9: 

Many thanks Rik for all the precisions, now it's perfectly clear, and I have no more excuses for being useless. I'll look closely to the guideline as soon as possible.

Hugo Raguet <snake>
Mon 20 Nov 2017 07:31:06 PM UTC, comment #8: 


>> 1) the function is creating triangles and coloring them. The most straightforward form should then provide one color per triangle in arguments. What should be the expected behavior when giving one color per vertex ? I guess coloring each triangle with the color of the first vertex defining it makes sense (and seems to be the implemented behavior), but this should appear in the documentation.


Matlab, and hence Octave, implements trisurf through the patch function.  The coloring of patches is very complicated.  See https://www.mathworks.com/help/matlab/creating_plots/how-patch-data-relates-to-a-colormap.html.  From this documentation you can see that either one color per face, or one color per vertex are acceptable.  The test results from Matlab show that it is using one color per vertex though, so Octave should do the same.  In particular, the default "FaceColor" property is set "flat" which means the color of face is determined by the first vertex.  However, if the "FaceColor" property is set to "interp" then the color varies smoothly across the face of the polygon as it moves from the color at vertex1 to the color at vertex2 and vertex3.  In this case, it is quite important to be able to specify the colors of all vertices, not just one color per face.

>> 2) I agree that "extending the input validation to accept either scenario is not hard", but it has currently not been done (Octave 4.2.1 in Linux) and this is what caused this thread in the first place.


It hasn't been done, because no one confirmed the correct behavior with Matlab until 1 day ago.

>> Is any developer planning to do it? I'd be glad to do it myself but I do not know yet where I should start to contribute, and if it is even worth for such small changes.


It is always very helpful, and very useful, if an initial patch is supplied for a bug report.  Polishing of a patch by developers is easier than writing a patch from scratch.  And it is very likely that a 4.2.2 bug fix release will be put out at the end/start of the year so there is still time to get any fix posted here into that release.

For some general guidelines see http://wiki.octave.org/Contribution_guidelines.

Rik <rik5>
Group administrator
Mon 20 Nov 2017 05:08:43 PM UTC, comment #7: 

I am a bit confused.

1) the function is creating triangles and coloring them. The most straightforward form should then provide one color per triangle in arguments. What should be the expected behavior when giving one color per vertex ? I guess coloring each triangle with the color of the first vertex defining it makes sense (and seems to be the implemented behavior), but this should appear in the documentation.

2) I agree that "extending the input validation to accept either scenario is not hard", but it has currently not been done (Octave 4.2.1 in Linux) and this is what caused this thread in the first place.

Is any developer planning to do it? I'd be glad to do it myself but I do not know yet where I should start to contribute, and if it is even worth for such small changes.

Hugo Raguet <snake>
Mon 20 Nov 2017 03:47:59 PM UTC, comment #6: 

Thanks for testing.  The error message is nice and clear that trisurf can either have one color per face or one color per vertex (as currently coded in Octave), but no other combinations.  Extending the input validation to accept either scenario is not hard.

Rik <rik5>
Group administrator
Mon 20 Nov 2017 06:59:56 AM UTC, comment #5: 


[x,y] = meshgrid(1:15,1:15);
tri = delaunay(x,y);
z = peaks(15);
trisurf(tri,x,y,z, 1:10)
colormap ('jet')
Warning: Error creating or updating Patch
 Error in value of property  FaceVertexCData
 Number of colors must equal number of vertices or faces


And empty 3d figure is shown


>> [x,y] = meshgrid (1:15,1:15);
tri = delaunay (x,y);
z = peaks (15);
h = trisurf (tri,x,y,z);
size (tri)
size (get (h, 'cdata'))
size (get (h, 'facevertexcdata'))

ans =

   392     3


ans =

     3   392


ans =

   225     1



Anonymous
Sun 19 Nov 2017 07:42:59 PM UTC, comment #4: 

Hi Rik

Sorry I do not have access to Matlab right now so I cannot directly help you, but don't you agree with me that the only problem comes from the fact the Octave source code for trisurf currently expects one color per vertex, while it should rather expect one color per triangle ?

Hugo Raguet <snake>
Sun 19 Nov 2017 02:55:46 AM UTC, comment #3: 

Can someone with access to Matlab run the test code from comment #1?  I'd like to see the error message.

Also, as an addendum, try


[x,y] = meshgrid (1:15,1:15);
tri = delaunay (x,y);
z = peaks (15);
h = trisurf (tri,x,y,z);
size (tri)
size (get (h, 'cdata'))
size (get (h, 'facevertexcdata'))




Rik <rik5>
Group administrator
Tue 31 Oct 2017 04:29:14 PM UTC, comment #2: 

Hello

similar problem here, Octave 4.2.1 on Linux.
Obviously, `trisurf` is supposed to specify one color per triangles. However, the source code expects one color per vertex, which is wrong:

...
        error ("trisurf: C must have 'numel (Z)' elements");
...
      error ("trisurf: TrueColor C matrix must be 'numel (Z)' rows by 3 columns");
...
    c = z(:);
...

However; when calling directly the primitive `patch` function (which is eventually called in trisurf.m, but with wrong arguments), all is well.
This might explain other `trisurf`related bugs, and seems to be easily fixed.

Hugo Raguet <snake>
Wed 23 Nov 2016 01:31:54 AM UTC, comment #1: 

What is the size of SSDisp?

Also, if you have access to Matlab, does the following work?


[x,y] = meshgrid(1:15,1:15);
tri = delaunay(x,y);
z = peaks(15);
trisurf(tri,x,y,z, 1:10)
colormap ('jet')


Maybe there is no input validation on the color input C, but that seems odd. 

Rik <rik5>
Group administrator
Fri 03 Jun 2016 09:03:02 AM UTC, original submission:  

Hi all,

I am running the trisurf function.

I am calling it like so:

trisurf(Triangles,Points(:,2),Points(:,3),Points(:,4),SSDisp);

'Triangles' is a 3xn vector of the triangluation
This points to rows in the 'Points' which is a vector where each row defines the corner points of each triangle.
'SSDisp' is a value at the centre of each triangle I have calculated.
If I try and run this in the current octave version (4.0) this gives an error 'error: trisurf: C must have 'numel (Z)' elements'

This is not in the octave documentation or explicitly in the MATLAB online docs either but MATLAB actually allows users to colour triangles in trisurf with a colour matrix that is the size of the number of triangles, This means the user does not need the colour vector to be the same size as the 'z' values. Picture attached

Currently the Trisurf code looks like this:


    handle = patch ("Faces", tri, "Vertices", [x(:), y(:), z(:)],
                    "FaceVertexCData", reshape (c, numel (c), 1),
                    varargin{:});


It will colour the triangles without an error if changed to:


    handle = patch ("Faces", tri, "Vertices", [x(:), y(:), z(:)],
                    "FaceVertexCData", c(:),
                    varargin{:});
    if (nargout > 0)


I.e this is not reshaping c (the colour vector) but just saying its a column vector.

The function works fine for me if I change this to the code I have added above. I have attached a picture of this working.

(Ps I am English so spell colour differently to Americans)

Cheers,

Tim

Anonymous

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #42484:  trisurf.patch added by snake (3KiB - text/x-patch)
file #37369:  SSDisp.PNG added by None (95KiB - image/png - Coloured correctly with the new code)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by snake (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by None (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-11-27 rik5 StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2017-11-25 snake Attached File- Added trisurf.patch, #42484
    2017-11-20 rik5 StatusNeed Info In Progress
        Operating SystemMicrosoft Windows Any
    2016-11-23 rik5 StatusNone Need Info
        Release4.0.0 4.2.0
    2016-06-03 None Attached File- Added SSDisp.PNG, #37369

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code