Update to latest google3 version by jmr · Pull Request #312 · google/s2geometry

jmr · 2023-04-24T10:48:05Z

Use absl::flags and absl::log instead of gflags/glog
Use ABSL_UNREACHABLE, requires abseil/abseil-cpp@6a87605
New edge/circle intersection ordering predicates
New functions GetUVCoordOfEdge and GetIJCoordOfEdge
S2PolylineAlignment: Use Norm instead of Norm2
S2Polygon::DestructiveUnion: Use priority_queue internally
Update CONTRIBUTING.md with more detailed instructions
Replace const char * with absl::string_view
Replace most gtl::dense_hash_set uses with absl::flat_hash_set
Replace most std::set and std::map with absl::flat_hash_set
and flat_hash_map

* Use absl::flags and absl::log instead of gflags/glog * Use ABSL_UNREACHABLE, requires abseil/abseil-cpp@6a87605 * New edge/circle intersection ordering predicates * New functions `GetUVCoordOfEdge` and `GetIJCoordOfEdge` * S2PolylineAlignment: Use Norm instead of Norm2 * S2Polygon::DestructiveUnion: Use priority_queue internally * Update CONTRIBUTING.md with more detailed instructions * Replace `const char *` with `absl::string_view` * Replace most `gtl::dense_hash_set` uses with `absl::flat_hash_set` * Replace most `std::set` and `std::map` with `absl::flat_hash_set` and `flat_hash_map`

Update to latest google3 version * Use absl::flags and absl::log instead of gflags/glog * Use ABSL_UNREACHABLE, requires abseil/abseil-cpp@6a87605 * New edge/circle intersection ordering predicates * New functions `GetUVCoordOfEdge` and `GetIJCoordOfEdge` * S2PolylineAlignment: Use Norm instead of Norm2 * S2Polygon::DestructiveUnion: Use priority_queue internally * Update CONTRIBUTING.md with more detailed instructions * Replace `const char *` with `absl::string_view` * Replace most `gtl::dense_hash_set` uses with `absl::flat_hash_set` * Replace most `std::set` and `std::map` with `absl::flat_hash_set` and `flat_hash_map`

jmr · 2023-04-25T07:39:58Z

@MBkkt

MBkkt · 2023-04-25T07:56:24Z

CMakeLists.txt

-    add_definitions(-DS2_USE_GFLAGS)
-endif()
+# Don't output anything for LOG(INFO).
+add_definitions(-DABSL_MIN_LOG_LEVEL=1)


Looks like now impossible to disable s2 log, flags, asserts without patching

Is that going to be a problem for you?

The main reason there was S2_USE_GFLAGS was in case someone didn't have it installed. Since it's all part of absl, it's less important now, but I can imagine people wanting to swap in another flags/logging package or save the size.

I think -DABSL_MIN_LOG_LEVEL=99 -DDABSL_MAX_VLOG_VERBOSITY=0 will disable logging. I don't think there's currently an "absl flags are just constants" option.

I don't know, just noticed it can be big behavior change

We'll see if anyone complains.

src/s2/s2cell.h

src/s2/s2cell_id_test.cc

MBkkt · 2023-04-25T08:13:55Z

src/s2/s2coords.h

+// The XYZ -> UV conversion is a single division per coordinate, which is
+// promised to be at most 0.5*DBL_EPSILON absolute error for values with
+// magnitude less than two.
+constexpr double kMaxXYZtoUVError = 0.5 * DBL_EPSILON;


inline constexpr? Or maybe at least todo if you're using C++14
https://quuxplusone.github.io/blog/2022/07/08/inline-constexpr/

There's an implicit TODO for all these C++17 features when we bump the minimum version.

MBkkt · 2023-04-25T08:14:59Z

src/s2/s2edge_clipping.h

-extern const double kFaceClipErrorRadians;
-extern const double kFaceClipErrorUVDist;
-extern const double kFaceClipErrorUVCoord;
+constexpr double kFaceClipErrorRadians = 3 * DBL_EPSILON;


Same for all these variables

src/s2/s2edge_crossings_test.cc

MBkkt · 2023-04-25T08:25:34Z

src/s2/s2polygon.cc

+  using PriorityQueue = priority_queue<Pair, vector<Pair>, greater<Pair>>;
+  PriorityQueue queue;  // Least vertices is `top`.
+  for (size_t i = 0; i < polygons.size(); ++i) {
+    queue.emplace(polygons[i]->num_vertices(), i);


Size of polygons doesn't change here, so you can use address instead of compute it in loop with index

That makes the resulting polygon depend on the addresses of the input polygons, which some clients complained about.

Hmm I probably should looked how union polygons work.
Ok

Hmm I understand, above exist comment, I missed it

But wait, addresses of polygon in vector is monotonically increasing.
So I think this comment was about elements itself (addresses of polygons, not their cell in vector)

I see. Yeah, the comment was about S2Polygon *. You want to use a unique_ptr<S2Polygon> *? Yeah, that's probably better since it can't be confused with num_vertices.

I need the index to put the polygon back into the array. Subtracting the pointers to get the index seems a bit roundabout.

We'll probably do this in a future release.

src/s2/s2polygon.cc

src/s2/s2shapeutil_shape_edge_id.h

rsned · 2023-04-30T19:58:01Z

src/s2/s2polyline_alignment_test.cc

+  // right angles, that would get a different matching of points for distance
+  // cost versus squared distance cost. If we had used squared distance for the
+  // cost the path would be {{0, 0}, {1, 0}, {2, 0}, {3, 1}, {3, 2}}; See
+  // https://screenshot.googleplex.com/7eeMjdSc5HeSeTD for the costs between the


Should probably add a check for internal URL's for screenshot.googleplex.com

rsned · 2023-05-02T17:38:26Z

src/s2/s2predicates.cc

+  return ExactIntersectionOrdering(  //
+      ToExact(a), ToExact(b), ToExact(c), ToExact(d), ToExact(m), ToExact(n));
+
+  return 0;


There two returns here (line 913 && 916)

Shocked the linter doesn't catch this, I'll get it fixed for the next roll. Glad to see you're still contributing btw =D

jmr added 2 commits April 20, 2023 11:10

MBkkt reviewed Apr 25, 2023

View reviewed changes

jmr merged commit d184302 into google:master Apr 27, 2023

jmr deleted the update-2023-04-20 branch April 27, 2023 07:35

rsned reviewed Apr 30, 2023

View reviewed changes

rsned reviewed May 2, 2023

View reviewed changes

chenrui333 mentioned this pull request Jan 28, 2024

s2geometry 0.11.0 Homebrew/homebrew-core#161134

Closed

Conversation

jmr commented Apr 24, 2023

Uh oh!

jmr commented Apr 25, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants