patchGNU Octave - Patches: patch #8912, add function reducepatch

 
 

patch #8912: add function reducepatch

Submitter:  Markus Mützel <mmuetzel>
Submitted:  Thu 18 Feb 2016 09:47:01 AM UTC
   
 
Category:  Core : new function Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  None Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Tue 09 Aug 2016 11:51:39 PM UTC, comment #13: 

I modified the patch to Octave standards and pushed it here (http://hg.savannah.gnu.org/hgweb/octave/rev/654de580bdb3).

Rik <rik5>
Group administrator
Wed 20 Jul 2016 08:07:05 PM UTC, comment #12: 

Demo works fine in Windows.

Philip Nienhuis <philipnienhuis>
Group Member
Tue 19 Jul 2016 10:40:35 AM UTC, comment #11: 

Sorry, I uploaded the wrong patch. The texinfo line still had the trailing space. Must have forgotten to hit save in the right tree...
Please, use the attached patch instead.

(file #37947)

Markus Mützel <mmuetzel>
Group administrator
Mon 18 Jul 2016 05:56:24 PM UTC, comment #10: 

The attached patch should apply cleanly on top of file #37936 in patch #8856 "reducevolume".

(file #37937)

Markus Mützel <mmuetzel>
Group administrator
Mon 11 Jul 2016 05:19:09 PM UTC, comment #9: 

Thank you, Philip, for testing several of my patches.

They will definitely interfere with each other in the @seealso lines of "light.m" and "isosurface.m", probably also in "plot.txi" and maybe some places else as well. I already wondered how to best proceed with this.

When I have "green lights" from you or someone else for which patches are going to be included in 4.2.0, I will start to give them an order.

Markus Mützel <mmuetzel>
Group administrator
Fri 08 Jul 2016 05:27:36 PM UTC, comment #8: 

Thx, I'll try later. As it is pure .m file stuff I can patch my cross-build (running ATM) by hand.

I applied several of your patches to my local build and found that they interfere, usually in the @seealso{...} lines.

I expect it to be a temporary situation so no issue, but you might start thinking about an order in which to apply the patches for e.g., isocaps, reducepatch, reducevolume, lighting, material, ... (I forgot one I guess)
I hope all of these can incorporated in octave-4.2.0

Philip Nienhuis <philipnienhuis>
Group Member
Fri 08 Jul 2016 04:14:10 PM UTC, comment #7: 

Philip, thank you for testing this patch. I guess I should not test the "verbose" flag too excessively.

Attached please find a changeset where I removed that flag from the tests.

(file #37774)

Markus Mützel <mmuetzel>
Group administrator
Fri 08 Jul 2016 03:29:33 PM UTC, comment #6: 

Markus,
"test reducepatch" produces a lot of verbose output that isn't required. Apparently the "verbose" flag is set somewhere unintentionally?
Could you tame down reducepatch a bit, please?

Philip Nienhuis <philipnienhuis>
Group Member
Wed 06 Jul 2016 07:16:44 PM UTC, comment #5: 

Lachlan, attached please find the reducepatch function within a proper patch.
Since the latest version of "__unite_shared_vertices__" was already pushed in 634fbedbfb5b for bug #46946, it is no longer included here.

(file #37736)

Markus Mützel <mmuetzel>
Group administrator
Wed 06 Jul 2016 11:39:11 AM UTC, comment #4: 

Sorry about the long delay in replying.

The rule about indenting broken lines is that the second part of the item starts at the same column as the first.  For example,


function_call (a, b,
               c, d)



  if (variable.long_component
              .next_component (2))


Lachlan Andrew <lachlan>
Thu 25 Feb 2016 12:43:19 PM UTC, comment #3: 

"__unite_shared_vertices__" from the original comment was so slow and so memory hungry that I could not help but try to improve on it.
It looks like this is one of the cases where a loop performs better than the vectorized version (with operations on many redundant matrix elements).

(file #36457)

Markus Mützel <mmuetzel>
Group administrator
Mon 22 Feb 2016 10:13:01 AM UTC, comment #2: 

Thank you, Lachlan, for your help in improving the patch.

Attached please find the patch in which I hopefully addressed all the issues you mentioned.

Wrt splitting lines: Is there a coding style defining how much the broken lines should be indented? I used twice the "normal indentation" unless some other indentation imho better matched the parts of the broken lines. Is this OK?

Can you (or someone else) please give a hint how I should proceed with "__unite_shared_vertices__" from the original comment? Should I re-upload it for the patch in bug #46946?

(file #36420)

Markus Mützel <mmuetzel>
Group administrator
Fri 19 Feb 2016 10:37:13 PM UTC, comment #1: 

Thanks, Markus.

I have a few trivial comments on coding style:
- Please avoid long lines.  I think the recommended limit is
  about 72 or 76 chars, although I normally use up to 80.
  Strings can be broken by ["splitting them ", "into arrays ",
  "like this"], and breaking at commas.

- For the for loop, please place the "var =" inside
  the parentheses.

- Please place spaces around "+"; they're missing in a few
  "...+1" expressions.

In the doc string, it would be nice for the first sentence to be complete (including "the" and full stop), and perhaps be more informative. Perhaps: "Reduce the number of patch faces and vertices in a <what type of object?  the output of a particular command/package?>, while retaining the patch outline."

Also, "+= 1" is currently twice as fast as "++".  You may want to avoid ++.

(I can't guarantee that these changes will help the patch to be applied by someone authorised to do that, but I hope they may.)

Lachlan Andrew <lachlan>
Thu 18 Feb 2016 09:47:01 AM UTC, original submission:  

This patch is to add the function "reducepatch" to Octave.

I think it covers almost all use cases that are covered in MathWorks documentation of the function.

The only missing bit is probably that it relies on the given patches to consist of triangles only. Otherwise, it fails with a specific error. Since the current implementation of "isosurface" seems to give patches that consist of only triangles, this limitation should not be too hard. And it should be reasonably easy to implement the triangulation later if really needed.

The approach taken for the reduction is the one discribed e.g. here [1] (simple shortest-edge-midpoint cost/placement strategy).
There are probably better ways (taking e.g. "curvature" at the vertices into consideration). And Matlab seems to do it in a more clever way. However, the taken approach is easy and more or less fast (and probably a good start for later improvement?).

Vertices of neighboring faces must be shared for the reduction to give a correct result. To detect and unite shared vertices, I took the same approach as for "isosurface" in bug #46946 with the same considerations that there is probably a more efficient way to do it.

Since "isosurface" is using the same functionality, I put this part of the code into the function "__unite_shared_vertices__" which should be put into the common private folder of the two functions "reducepatch" and "isosurface".

I am going to update the patch in bug #46946 to use this private function, too. Should I attach "__unite_shared_vertices__" to the other bug, too? Or do you deem it better to maintain the function only in this patch and give a link to it in the bug #46946 ?

[1] http://libigl.github.io/libigl/tutorial/tutorial.html#meshdecimation


Markus Mützel <mmuetzel>
Group administrator

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #37947:  reducepatch_v3b.patch added by mmuetzel (19KiB - text/x-diff)
file #37937:  reducepatch_v3.patch added by mmuetzel (19KiB - text/x-diff)
file #37774:  reducepatch_v2.patch added by mmuetzel (19KiB - text/x-diff)
file #37736:  reducepatch.patch added by mmuetzel (20KiB - text/x-diff)
file #36457:  __unite_shared_vertices__.m added by mmuetzel (3KiB - text/x-objective-c)
file #36420:  reducepatch.m added by mmuetzel (17KiB - text/x-objcsrc)
file #36378:  reducepatch.m added by mmuetzel (16KiB - text/x-objcsrc)
file #36379:  __unite_shared_vertices__.m added by mmuetzel (3KiB - text/x-objcsrc)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by mmuetzel
  • -email is unavailable- added by lachlan (Posted a comment)
  • -email is unavailable- added by mmuetzel (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 logged-in users can vote.

     

    Follow 12 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-08-09 rik5 StatusNone Done
        Open/ClosedOpen Closed
    2016-07-19 mmuetzel Attached File- Added reducepatch_v3b.patch, #37947
    2016-07-18 mmuetzel Attached File- Added reducepatch_v3.patch, #37937
        Carbon-Copy- Added philipnienhuis
    2016-07-08 mmuetzel Attached File- Added reducepatch_v2.patch, #37774
    2016-07-07 lachlan CategoryNone Core : new function
    2016-07-06 mmuetzel Attached File- Added reducepatch.patch, #37736
    2016-02-25 mmuetzel Attached File- Added _unite_shared_vertices_.m, #36457
    2016-02-22 mmuetzel Attached File- Added reducepatch.m, #36420
    2016-02-18 mmuetzel Attached File- Added reducepatch.m, #36378
        Attached File- Added _unite_shared_vertices_.m, #36379

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code