Fix the user data of geometry in voronoi diagram.

[whuaegeanse]

(1) Use a heap variable pointer as user data of polygon of voronoi diagram;
(2) Set user data for polygon of voronoi diagram.

[dbaston]

Thanks for the fix, @whuaegeanse . Would you consider adding a test case in https://github.com/libgeos/geos/blob/master/tests/unit/triangulate/VoronoiTest.cpp ?

[whuaegeanse]

OK, I will add one.

[kenash0625]

and memory management will be ugly,i mean,caller should free memory libgeos allocated
Coordinate *pcoord = (Coordinate *)polys->getGeometryN(i)->getUserData();
delete pcoord;

[whuaegeanse]

@kenash54845 Yes, you are right. The user data is a pointer, which can not store a pointer of the local variable. Any solution to fix this?

[kenash0625]

libgeos wont fix this,i guess

[whuaegeanse]

@kenash54845 I will add a test for this and have to modify some other tests.

[kenash0625]

@whuaegeanse have u tested whether userdata is null or not? setUserData should be moved several lines down,i think,
\src\triangulate\VoronoiDiagramBuilder.cpp

		if(clipEnv.contains(g->getEnvelopeInternal()))
		{
			result.reset( g->clone() );
      // TODO: check if userData is correctly cloned here?
		}
		else if(clipEnv.intersects(g->getEnvelopeInternal()))
		{
			result.reset( clipPoly->intersection(g) );
			
		}

		if(result.get() && !result->isEmpty() )
		{			result->setUserData(((Geometry*)g)->getUserData()); // TODO: needed ?

			clipped->push_back(result.release());
		}

here is code snippet i m using for test

	geos::geom::CoordinateArraySequence seq;
	geos::geom::Envelope env;
	geos::triangulate::VoronoiDiagramBuilder builder;
	const geos::geom::GeometryFactory& geomFact(*geos::geom::GeometryFactory::getDefaultInstance());
	std::auto_ptr<geos::geom::GeometryCollection> polys;
	
	geos::geom::Coordinate coordrun(0,0);
	seq.add(coordrun);
	env.expandToInclude(coordrun);

	geos::geom::Coordinate coordrun2(10, 0);
	seq.add(coordrun2);
	env.expandToInclude(coordrun2);

	geos::geom::Coordinate coordrun3(5,10);
	seq.add(coordrun3);
	env.expandToInclude(coordrun3);
	

	builder.setSites(seq);
	builder.setTolerance(0);
	builder.setClipEnvelope(&env);
	polys = builder.getDiagram(geomFact);
	for (std::size_t i = 0; i < polys->getNumGeometries(); ++i)
	{
		const geos::geom::Point *pcoord = (const geos::geom::Point *)(polys->getGeometryN(i)->getUserData());
		if (pcoord == nullptr)
		{
			cout << "??" << endl;
		}
	}

[pramsey]

OK, I see two PRs for this, which one is going forward?
In Java world the ownership of the user data is not really an issue as GC will clean it up once orphaned.
In the GEOS case we seem to be counting on the setter of the userData to retain ownership and clean it up when finished. That model breaks down when we start setting userData ourselves behind the backs of the users.
This "fix" seems like it'll open up a bunch of problems to end users, who will be leaking memory all over the place.
We could change the _userData to be a unique_ptr, but then we have a new problem for all the existing users of _userData who get their user data cleaned up when the geometry is deleted.
The whole idea of a void pointer feels blunt force, a map would be so much nicer... but even then the question of ownership arises, unless the _userData remains a user owned and a new map of geos-added flavourings that are managed by geos is added to hold stuff like this.

[dbaston]

Returning the coordinate through userData seems like a hack in the first place. Maybe the VoronoiDiagramBuilder could be modified to produce something like std::vector<std::pair<Geometry*, Coordinate>> instead?

[pramsey]

Could do. Still leaves us w/ the C API to deal with, in case anyone is hoping to get access to those coordinates through that route...

[pramsey]

I think this is a worthy goal, but this approach isn't it. Something that uses @dbaston's std::pair return idea and also figures out how to expose that via the CAPI would be a complete solution.