umbrella icon indicating copy to clipboard operation
umbrella copied to clipboard

[@thi.ng/geom-resample] Angular resolution resample unimplemented

Open blainelewis1 opened this issue 2 years ago • 2 comments

I noticed there is a parameter to resample to angular resolution: https://github.com/thi-ng/umbrella/blob/develop/packages/geom-api/src/sample.ts

However, I am not getting the expected result when I use theta as a parameter:

resample([[0,0], [0,1],[1,1], [1,1], [1,1], [1,2]], { theta: Math.PI / 6 })

Expected:

[[0,0], [0,1],[1,1], [1,2]]

Received:

[[0,0],[0,0.15],[0,0.3],[0,0.44999999999999996],[0,0.6],[0,0.7499999999999999],[0,0.8999999999999999],[0.04999999999999982,1],[0.19999999999999996,1],[0.34999999999999987,1],[0.4999999999999998,1],[0.6499999999999999,1],[0.7999999999999998,1],[0.9500000000000002,1],[1,1.1],[1,1.2500000000000004],[1,1.4000000000000004],[1,1.5500000000000007],[1,1.7000000000000006],[1,1.850000000000001],[1,2]]

I believe this is just the default behaviour and the resample by angular resolution is unimplemented, see:

https://github.com/thi-ng/umbrella/blob/develop/packages/geom-resample/src/simplify.ts

and

https://github.com/thi-ng/umbrella/blob/develop/packages/geom-resample/src/resample.ts

Neither mention theta which I think means the feature was documented but not implemented.

blainelewis1 avatar Apr 19 '22 15:04 blainelewis1

Hi @blainelewis1 - the theta option is only used for the sampling of circles, ellipses and arcs (because these're the only shapes where that option makes sense):

https://github.com/thi-ng/umbrella/blob/cfc8590807e23280cb6f3e929219217f7eaaaaac/packages/geom/src/vertices.ts#L152

https://github.com/thi-ng/umbrella/blob/cfc8590807e23280cb6f3e929219217f7eaaaaac/packages/geom-arc/src/sample.ts#L34

The reason the option is listed in the SamplingOpts interface is due to the polymorphic nature of operations in the main thi.ng/geom package, e.g. the vertices() implementations (and many others). Maybe the theta option should be moved from the base interface into a new/extended version (can't think of a good name right now though, SamplingOptsXXX)

In any way, for now I should add a more explicit note to the theta docstring to point out this sampling option will only be used by some shapes...

postspectacular avatar Apr 21 '22 14:04 postspectacular

I actually also just had this exact misunderstanding. I was trying to find a way of specifying that I wanted samples to be every d distance, but to have the sampler automatically increase resolution if the curvature of the path reached a certain threshold. I finally realized that this combination of constraints wasn't possible, but I did spend some time trying to figure it out.

JeffreyPalmer avatar Oct 29 '22 23:10 JeffreyPalmer