michalbrath / poly2tri

Automatically exported from code.google.com/p/poly2tri
Other
0 stars 0 forks source link

Ruby Bindings #9

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
poly2tri is brilliant.  I've created Ruby bindings, which are located at 
http://github.com/mieko/rbpoly2tri .  It includes the required C++ 
implementation in the package.  It's not amazingly well tested, but most of the 
surface area if covers is pretty well stressed by my application. (AKA, Works 
For Me!)

Assuming poly2tri is still active, I've started making a few memory reduction 
changes (basically, getting rid of unneeded copies).  If someone's around to 
check on them, I'd be happy to create a proper hg branch and isolate just those 
when I have the time.

Thanks,
Mike Owens

Original issue reported on code.google.com by mike%fil...@gtempaccount.com on 12 Oct 2010 at 7:50

GoogleCodeExporter commented 9 years ago
Great to hear you like Poly2Tri. Poly2Tri is active in that sense that any 
found bugs should be fixed, but it seems the first release was pretty stable :)

Did you want me to setup a repository for the Ruby bindings?

Original comment by thahlen@gmail.com on 13 Oct 2010 at 12:32

GoogleCodeExporter commented 9 years ago
I'm not amazingly familiar with hg, so for the bindings that's not required.  

There are some changes I need to make to poly2tri for the bindings, but before 
implementing them, I wanted to see if they could be done in a way that would be 
acceptable upstream.  I'm willing to work in hg if it makes it easier to get 
things pulled.  Perhaps you can advise me on the process that'd be preferred 
for these types of changes to the C++ implementation:

1) Getting the bail cases off of assert(false); When poly2tri is compiled 
without assertions (-DNDEBUG), my bindings just crash on invalid input like 
coincident points in the polyline.  I assume the exceptions were commented out 
and replaced with asserts for a reason (overhead?), but I'd like to know if 
/any/ solution would be acceptable in poly2tri proper (error callback, errno, 
bring back exceptions?) I'm pretty much forced to do this to have usable 
bindings.

2) There are quite a few areas which return pretty large structures by value, 
both internally and in the user-visible API.  Profiling showed a fair amount of 
time in the vector<Point*> copy-constructor.  A lot of these could be changed 
to const-refs to avoid the copy without changing source compatibility.  Unlike 
#1, this is just a wishlist type of thing.  Tt struck me because for the 
bindings, AFTER that copy, I unavoidably must copy/convert them yet again to 
get them into Ruby values, so I was trying to lighten up this bit.

Original comment by mike%fil...@gtempaccount.com on 13 Oct 2010 at 4:22

GoogleCodeExporter commented 9 years ago
I am maintaining the Java version and have forwarded this to Mason Green who 
has done the C++ port. 

1. In the case where you send in multiple points with the exact same coordinate 
the algorithm will fail. The only way around that for now is to validate the 
input before the triangulation.
If this happens with a polyline then the input wouldn't be a simple polygon?

Original comment by thahlen@gmail.com on 13 Oct 2010 at 7:44

GoogleCodeExporter commented 9 years ago
I guess the case you have might encounter is when a closed polyline start and 
end at the same point and both points are included in the point list. In the 
java implementation I do a simple check and fix for these cases since they seem 
common.

All other cases with multiple point with same coordinates fail and will require 
some pre validation.

Original comment by thahlen@gmail.com on 13 Oct 2010 at 7:56

GoogleCodeExporter commented 9 years ago
Thanks thahlen.  

I've found a (hacky) solution to problem #1 by basically unconditionally 
enabling asserts, and redefining "assert" in the build to a function that 
throws[1].  Not ideal, but doesn't require me to have a diverging copy of 
poly2tri's C++ implementation, or pre-filter every polygon that comes in.

Regardless, when Mason has a chance to look at this, the assert behavior does 
pose issues for bindings (vs. ports).  I don't know if this is a use-case 
anyone is interested in, though.

The issue is, as rbpoly2tri is a wrapper over the C++ implementation, poly2tri 
is compiled once (when the extension is built), so the users of the bindings 
can't readily leave it in "debug mode" while developing and "release mode" once 
all their bugs have been worked out.  As the maintainer of the bindings, I have 
to choose between:

  1. Enable asserts: When poly2tri is given an invalid polygon, it aborts the program.  This kills the entire VM, as I have no way to step in.

  2. Disable asserts:  rbpoly2tri passes along the invalid polygon, but poly2tri's asserts have been compiled down to NOPs, so it runs into uncharted territory and ends up with a segv.

I can live with the solution I'm using, but I have a bit of flexibility being 
an extension written in native code.  FFI, ctypes, or dlopen-style bindings in 
any language wouldn't be able to deal with this without choosing between the 
behaviors above.  This is also the reason I cannot just point users at poly2tri 
and say "install that, then install this".

Not a big deal, just something to consider.

[1] http://github.com/mieko/rbpoly2tri/blob/master/throw_assert/assert.h

Original comment by mike%fil...@gtempaccount.com on 13 Oct 2010 at 8:33

GoogleCodeExporter commented 9 years ago
Mike,

To address your second post above:

1) I have no problem replacing the asserts with error callbacks or a better 
method of catching the bail cases.

2) I would definitely appreciate any memory optimization contributions you 
would be able to make :)

3) Currently I'm a little busy with other aspects of my life and would not mind 
giving you access to the C++ trunk if you're interested in carrying the torch 
forward. I'm definitely not abandoning the project, although I'm time 
constrained at this point.

4) hg is fairly easy and *not* that much different than Git. The learning curve 
should be fairly low.

/Mason

Original comment by mason.gr...@gmail.com on 15 Oct 2010 at 2:52

GoogleCodeExporter commented 9 years ago
I've created a branch with some of the changes I'm working on here: 
http://code.google.com/r/mike-poly2tri-staging/

> 1) I have no problem replacing the asserts with error callbacks or a better 
method of catching the bail cases.

Trying to be as un-invasive to the structure as possible, what my branch is 
does is  bail with a new p2t_fail(), which depending on the configuration-time 
option --no-sanity-checks, expands to either an assert(0) or an inline function 
that throws.  

Figured it was a good middle ground: doesn't penalize users that have debugged 
their code and can guarantee all input is valid, but allows builds for cases 
like rbpoly2tri to catch input errors.

> 2) I would definitely appreciate any memory optimization contributions you 
would be able to make :)

I've backed out the changes I was doing here to make sure it was a totally 
thorough approach.  I created p2tdump (in the repo) so I can make sure the 
less-copying/const fixes have no effect at all on the output when I get back 
around to it.  My branch just has the ultra-low-hanging fruit, like avoiding a 
copy on the way out of GetTriangles.

> 3) not mind giving you access to the C++ trunk if you're interested in 
carrying the torch forward.

For now, I'd prefer to simmer in the branch:  I've only been using poly2tri for 
a few weeks, and the changes I'm making, while I believe are universal, may be 
biased toward making rbpoly2tri more viable.  For example,  don't think I'd 
honestly care about asserts vs. throws in this context if I were using poly2tri 
as a native C++ consumer.   In a few weeks when I'm done with my current 
project, the changes and bindings have had more stress on them, I'll get up 
with you.

Until then, I am trying to do my best to make changes independently pullable 
(though some intermingle: To had add p2tdump, I had to bring waf up to 
something more recent just to be working with something that the current waf 
documentation makes sense with.)

Thanks again guys for working on poly2tri.

-M

(BTW please feel free to mark this defect closed/fixed as I've already pull it 
off-topic.)

Original comment by mike%fil...@gtempaccount.com on 16 Oct 2010 at 3:53

GoogleCodeExporter commented 9 years ago
Oki I marked this as fixed.

Nice to see you are using the lib for a game :-)

Original comment by thahlen@gmail.com on 18 Oct 2010 at 10:26

GoogleCodeExporter commented 9 years ago
Hope you don't mind but I added a link to your Ruby bindings on the front page 
;)

Original comment by thahlen@gmail.com on 18 Oct 2010 at 11:52