Offset not exposed on Circle, though present on SAT implementation

[BobGneu]

Good day,

I have recently been working with the Circle bodies and have a case where the offset value would be very helpful.

https://github.com/jriecken/sat-js/blob/master/SAT.js#L291-L294

I can have a PR in for this over the weekend, just checking to see if there was a philosophical difference in goals or any other impediment in exposing it prior.

[Prozi]

Good day,

I have recently been working with the Circle bodies and have a case where the offset value would be very helpful.

https://github.com/jriecken/sat-js/blob/master/SAT.js#L291-L294

I can have a PR in for this over the weekend, just checking to see if there was a philosophical difference in goals or any other impediment in exposing it prior.

hello

strange since Circle extends SAT.Circle

no philosophical reason of not exposing it

if you have pr with a fix feel free to submit it

thank you

[BobGneu]

It looks like it is rooted in the typedef upstream, SATs signatures don't reflect the fact that its exposed.

I have validated tests, build, and local execution with start. Looks good to me. May play around in the local execution to include an offset circle in the test cases there.

Is there any documentation that would be helpful?

[BobGneu]

Quick update

So, looks like the calculations for the aabb of the circle are not taking the offset value into account. I am still debugging, More details to follow.

[Prozi]

Quick update

So, looks like the calculations for the aabb of the circle are not taking the offset value into account. I am still debugging, More details to follow.

this looks like it's because

https://github.com/Prozi/detect-collisions/blob/master/src/bodies/circle.ts#L160-L167

please try overriding this function and adding offset to pos then it should work

don't know about rotation though

[Prozi]

I've created a branch for test purpouses of this issue

https://github.com/Prozi/detect-collisions/tree/issue-36

please check it out @BobGneu it seems if you run

yarn build
yarn demo

and open the tank demo http://localhost:4200/

if tank loads as box (50% chance) it has correct collisions

if it loads as circle (50% chance) seems like it should work, but doesnt, maybe the sat:

import {
  testCircleCircle,
  testCirclePolygon,
  testPolygonCircle,
} from "sat";

don't take offset of circle into account.

i am to tired to check today.

have a nice day

[Prozi]

I managed to implement offset for circles

@BobGneu please check v6.6.0

[BobGneu]

Thank you for getting it in there, will check bright and early tomorrow. Very appreciated.

[BobGneu]

Looks like the signature for setOffset is not exactly correct, SAT appears to expose setOffset such that it should return a Circle.

Something like this may be in order:

  setOffset(offset: Vector): Circle {
    // ...

    return this;
  }

Thoughts?

[Prozi]

Looks like the signature for setOffset is not exactly correct, SAT appears to expose setOffset such that it should return a Circle.

Something like this may be in order:

  setOffset(offset: Vector): Circle {
  // ...

    return this;
  }

Thoughts?

will fix this now

thanks

[Prozi]

please try v6.6.1 where I did as you mentioned above

[Prozi]

closing until something else wrong with offset on circle :)

feel free to reopen if something wrong