fix bug in PoissonDiskSampler, add test

[dyollb]

Fixed #184

Sampling only behaved reasonable when the mean edge length was close to 1, because the "rMinDist" was treated as if it where the squared min. distance (sqrt was taken).

This PR fixes this issue and adds a test.

• a 1x1 square disk is sampled with distance=0.1
• a 100x100 square disk is sampled with distance=10 (i.e., 100*0.1)

In both cases we expect a similar number of points (seed cannot be set from API).

Without the fix, I get approx. 650 (width=1) vs 10 (width=100) points.

[keenancrane]

LGTM. @nmwsharp Should we be worried about this being a breaking change, e.g., for algorithms or demos that assume the original behavior (even it was wrong) in setting parameters, etc.?

[nzfeng]

Also looks good to me -- thank you for adding tests too!

One thing I've been meaning to do is to expose the distance parameter itself like @dyollb suggested, since in most cases it doesn't make much sense to have the minimum distance be in terms of the mesh's edge length. (I originally coded it up this way as a hacky proxy to a good "visual sampling density".) What do you all think?

[dyollb]

Also looks good to me -- thank you for adding tests too!

One thing I've been meaning to do is to expose the distance parameter itself like @dyollb suggested, since in most cases it doesn't make much sense to have the minimum distance be in terms of the mesh's edge length. (I originally coded it up this way as a hacky proxy to a good "visual sampling density".) What do you all think?

I pushed a small change to expose the mean edge length (fixes issue #183 )

[nmwsharp]

Thank you for pointing this out, fixing it, and even adding a test!

Breaking change

This is definitely a bug, and it's somewhat-okay to make breaking changes for bugfixes. However, this is indeed the kind of change that would silently cause someone's code to stop working on update, and it's best to avoid those whenever possible.

How about we also take the opportunity to factor out these options into a little options struct, and additionally include an absoluteRadius option? Any callers will get a compile error and be forced to briefly look at the code and chose a new parameter, and we can also offer the more-useful world coordinates radius an option.

(if this sounds like a good idea but others don't have time, I'd be happy to make this change)

[dyollb]

How about we also take the opportunity to factor out these options into a little options struct, and additionally include an absoluteRadius option? Any callers will get a compile error and be forced to briefly look at the code and chose a new parameter, and we can also offer the more-useful world coordinates radius an option.

(if this sounds like a good idea but others don't have time, I'd be happy to make this change)

I added a struct for the options. Should we rename the options to something users might understand without reading the code? e.g.,

struct PoissonDiskOptions {
  double relativeRadius = 1.0;
  double absoluteRadius = -1.0;
  int numCandidates = 30;
  std::vector<SurfacePoint> pointsToAvoid;
  int relativeAvoidanceRadius = 1;
  bool use3DAvoidanceRadius = true;
};

[dyollb]

Unfortunately, the test is flaky

Expected: (n1) > (600), actual: 592 vs 600

I can try to modify the test so it is not flaky, or allow the user to set a seed for the random number generator. What do you think?

[dyollb]

I can try to modify the test so it is not flaky, or allow the user to set a seed for the random number generator. What do you think?

I now set the seed in the test. Number of samples is identical for the scaled/unscaled square disk.

[nzfeng]

Thanks @dyollb for adding the new feature! Agree that adding a struct for options is the way to go.

I've streamlined the struct to only use distances specified world-space units, and updated the docs.

The struct is now

struct PoissonDiskOptions {
  double r = 1.0;       // minimum distance between samples
  int kCandidates = 30; // number of candidate points chosen from the (r,2r)-annulus around each sample
  std::vector<SurfacePoint> pointsToAvoid;
  double rAvoidance = 1.0; // radius of avoidance
  bool use3DAvoidance = true;
};

I'm strongly in favor of using only absolute distances, to avoid confusion, and to let the user decide if they want to specify values in terms of edge length, etc. I've tested everything on my end, so if there's no other bugs/features we'd like to fix, I'll merge.

[dyollb]

LGTM. Maybe the options could have more expressive names, e.g., 'r' -> 'rsdius' or 'sample_distance', and 'rAvoidance' -> 'avoidance_radius' or 'avoidance_distance'.

[nzfeng]

I've just merged. Thanks again for your help!

[nmwsharp]

Thanks a bunch @dyollb and @nzfeng, this is great!! I just rebuilt the docs, they're updated to show this change https://geometry-central.net/surface/algorithms/surface_sampling/