CGAL / cgal

The public CGAL repository, see the README below
https://github.com/CGAL/cgal#readme
Other
4.94k stars 1.38k forks source link

Two planes compare equal, to_2d produces different results for the same point. #5619

Open sbrian23 opened 3 years ago

sbrian23 commented 3 years ago

Issue Details

Two planes compare equal, but produce different results with to_2d.

I think Plane_3(0, 0, 1, 0) and Plane_3(0, 0, 2, 0) should be equivalent, and == agrees, but it seems like the y coordinate is scaled inversely by the magnitude of the third argument of the plane used.

The code below produces this output for me.

a: 1 1
b: 1 0.5
int main(): Assertion `a == b' failed.
Aborted

Could you let me know if I am confused, or if this looks like a bug?

Source Code

#include <CGAL/Exact_predicates_exact_constructions_kernel.h>

int main() {
  typedef CGAL::Exact_predicates_exact_constructions_kernel               EPEK;

  assert(EPEK::Plane_3(0, 0, 1, 0) == EPEK::Plane_3(0, 0, 2, 0));
  EPEK::Point_2 a = EPEK::Plane_3(0, 0, 1, 0).to_2d(EPEK::Point_3(1, 1, 1));
  EPEK::Point_2 b = EPEK::Plane_3(0, 0, 2, 0).to_2d(EPEK::Point_3(1, 1, 1));
  std::cout << "a: " << a << std::endl;
  std::cout << "b: " << b << std::endl;
  assert(a == b);

  return 0;
}

Environment

sbrian23 commented 3 years ago

I guess this is due to the Plane_3(0, 0, 2, 0) not being normalized.

What's a bit confusing is that this requirement for a normalized plane isn't documented, and this kind of unnormalized plane is being produced by Plane_3(a, b, c) for a, b, c are Point_3.

sloriot commented 3 years ago

IIRC, it's giving coordinates related to base1() and base2() and the way those vectors are defined is not documented. In practice I think they are simply defined by coordinates permutations without normalization. So you have a scaling of the 2D points if the plane normal is not a unit vector.

sbrian23 commented 3 years ago

Shall I put in a PR to update the documentation? :)

lrineau commented 3 years ago

I do not know what to say. CGAL objects from the kernel that compare equal according to == are only geometrically equal. But their member functions can return different values from two equivalent objects. For two CGAL::Plane_3 that compare equal, their member functions a(), b(), c(), d(), orthogonal_vector(), base1(), and base2() can return different values. And all the member functions as well. That is not considered as a bug.

Maybe operator== should have been about comparing the data members and there could have been another way to compare objects geometrically. But it is that way, and it will not change.

lrineau commented 3 years ago

Shall I put in a PR to update the documentation? :)

What would it be? Only about Plane_3::to_2d(point)? As I said, that behavior is common to all member functions of all CGAL kernel objects.

sbrian23 commented 3 years ago

Also, is there then a precise way to project from 3d to 2d?

Let's say that we have two planes that are geometrically equal, but defined by different triples.

In this case, these planes are differently unnormalized, so we cannot project them together onto the same 2d space.

We can normalize the planes, but if this involves an approximation due to sqrt, how can we expect the projection of the points to agree?

Is there any good way around this problem?

sbrian23 commented 3 years ago

Ah, I discovered Nef_S2/Normalizing.h which seems to do the trick for exact normalization of Plane_3. (Although I don't see documentation for it, which makes me wonder if I should be relying on it)

I think there might be two or three useful additions to the Plane_3 documentation.

(a) the Plane_3 constructors do not produce normalized planes, (b) to_2d and to_3d expect normalized planes, (c) and a note on best practice for normalizing planes.

I'm happy to put in a PR to do that, if you think it would be a reasonable addition.

sbrian23 commented 3 years ago

Actually, I think (b) should be that to_2d and to_3d require normalized planes for geometrical use-cases, but not for topological use-cases.

sloriot commented 3 years ago

Note that the doc says "The following functions provide conversion between a plane and CGAL's two-dimensional space. The transformation is affine, but not necessarily an isometry. This means, the transformation preserves combinatorics, but not distances."

sbrian23 commented 3 years ago

That's true, but it doesn't talk about being able to preserve distances by using a normalized plane.

Unless being able to use a normalized plane only works by accident, and there should really be no reliable way to convert from two geometrically equal planes to the same two-dimensional space?

anishsingh935 commented 3 years ago

Hi Can I try once to fix this issue if I allow, I know what is actually creating problem.

sloriot commented 3 years ago

Except if you use a kernel with exact sqrt, it will always be an approximation. We can probably add a note about normalizing that will reduce the deviation but that's it I would say.

sloriot commented 3 years ago

@anishsingh935 what is the fix you want to propose? "Normalizing" the vector each time prior to the call to the function is not something we want I think.

sbrian23 commented 3 years ago

I think the important thing is to make the user aware of how it works, so that they can prepare the plane appropriately, or find an alternative to to_2d and to_3d -- I wouldn't change the implementation. :)

I was hoping that the gcd based approach in Nef_2S/Normalizing.h would provide an exactly normalized plane, but perhaps I have misunderstood how it works?

sloriot commented 3 years ago

Nef_S2 means Nef polyhedra on the sphere. The normalization is not something working in general for planes.

sbrian23 commented 3 years ago

It's based on dividing the plane components by their gcd -- I think this should work on planes based on rationals, at least.

Actually, shouldn't that be all of the planes cgal supports?

I guess I'm misunderstanding something here -- but I'd like to understand if I'm misapplying this technique.

sloriot commented 3 years ago

Dividing by the gcd does not make a vector a unit vector but it makes it canonical.

sbrian23 commented 3 years ago

Ah, I see.

Are canonical vectors sufficient for to_2d and to_3d to map to the same 2d space?

sloriot commented 3 years ago

No you really need to normalize the vector to get distances in 2D and projected in 3D that are almost identical.

sbrian23 commented 3 years ago

Sorry -- I should have spoken more precisely. :)

Ok, but the canonical forms of the vectors would agree, so their projections would be equally biased, and agree topologically?

Which means you could project two faces in the equivalent 3d planes to 2d, take the union, and then project back?

sloriot commented 3 years ago

If normal vectors are identical then yes. But no need to normalize for that, simply pick one of the two planes and assigned it to the other plane (updating d). If the planes are the same, then simply make them identical p2=p1

anishsingh935 commented 3 years ago

If normal vectors are identical then yes. But no need to normalize for that, simply pick one of the two planes and assigned it to the other plane (updating d). If the planes are the same, then simply make them identical p2=p1

I think this would work

anishsingh935 commented 3 years ago

@anishsingh935 what is the fix you want to propose? "Normalizing" the vector each time prior to the call to the function is not something we want I think.

I thought if we normalize the vector then it would work fine. ok thanks for suggesting I am trying more to solve this issue

sbrian23 commented 3 years ago

Yes, deduplicating the planes is a good strategy where workable.

I'll see if I can come up with a minimal addendum to the docs so that what we discussed here should be obvious to future readers. :)