Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PIXI.Polygon#isClockwise Add the possibility to clear the cached result #8855

Closed
1 of 6 tasks
dev7355608 opened this issue Feb 4, 2023 · 4 comments
Closed
1 of 6 tasks
Assignees
Labels
api Issues related to the API used by Mod Devs bug Functionality which is not working as intended canvas Issues related to the PIXI canvas, rendering, and other WebGL functions.

Comments

@dev7355608
Copy link

What happened?

/**
 * Test whether the polygon is oriented clockwise.
 * Clockwise means that tracing the points of the polygon in order will, on average,
 * trace a clockwise circle. Formally, clockwise means the signed area of the polygon is positive.
 * @type {boolean}
 */
Object.defineProperty(PIXI.Polygon.prototype, "isClockwise", { get: function() {
  if ( this._isClockwise !== undefined ) return this._isClockwise;
  if ( this.points.length < 6 ) return undefined;
  return this._isClockwise = this.signedArea() > 0;
}});

The points of the polygon are mutable. So caching the orientation can lead to problems:

p = new PIXI.Polygon(0, 0, 1, 0, 0, 1);
console.log(p.isClockwise); // true
p.points = [0, 0, 0, 1, 1, 0];
console.log(p.isClockwise); // true

Also I don't think it makes sense to return undefined if there are less than 3 points. This is just one of many cases of degenerate polygons that do not have a well-defined orientation.

new PIXI.Polygon(0, 0, 0, 0).isClockwise // undefined
new PIXI.Polygon(0, 0, 0, 0, 0, 0).isClockwise // false

A polygon without well-defined orientation certainly doesn't have a clockwise orientation, so it makes sense to return false.

What ways of accessing Foundry can you encounter this issue in?

  • Native App (Electron)
  • Chrome
  • Firefox
  • Safari
  • Other

Reproduction Steps

.

What core version are you reporting this for?

Version 11 Prototype 1 (build 292)

Relevant log output

No response

Bug Checklist

  • The issue occurs while all Modules are disabled
@dev7355608 dev7355608 added the bug Functionality which is not working as intended label Feb 4, 2023
@aaclayton aaclayton added api Issues related to the API used by Mod Devs canvas Issues related to the PIXI canvas, rendering, and other WebGL functions. labels Feb 5, 2023
@aaclayton aaclayton added this to the Version 11 - Prototype 2 milestone Feb 5, 2023
@caewok
Copy link

caewok commented Feb 5, 2023

As a general rule, I don't think it is a good idea to treat polygons or other shapes as mutable. It would be preferable to return a new polygon instead of mutating it in many (maybe not all) cases. In my opinion, it complicates the code to have to assume the polygon will just mutate at will.

Testing for orientation is non-trivial, so checking repeatedly will have a performance impact. Orientation is particularly important for using polygons with holes, so as Foundry starts to rely on more complex geometric manipulations, this will become more of a concern.

As Foundry already adds an addPoint method, why not reset the cache if points are added through an established method? Otherwise, expect that if you modify the polygon points directly, you are expected to clear the cache. Maybe a clearCache or something similar here would be appropriate.

As for what to return when the polygon has 0, 1, or 2 points, I do not agree with returning false. If I ask if a polygon is oriented "clockwise," I would expect that if the answer is false, the polygon is "counterclockwise." This is necessary, again, to determine if a polygon represents a hole. Returning false whenever the polygon has less than 3 points makes all such polygons holes. Returning undefined is not perfect, but at least it is testable and distinct from true/false. The preferred approach would probably be to return an enumerated value, where CCW is 0, CW is 1, and "other" might be -1. Ideally, the same enumeration would be used by orient2dFast and anywhere else orientation is tested.

@dev7355608
Copy link
Author

I agree, caching the result would be preferable if possible, but PIXI.Polygon doesn't put any restrictions on its points. Foundry cannot change that without breaking pixi's API. I guess Foundry could add something like this to the docs of isClockwise: "If you manipulated the points since the last time you called isClockwise be means other that X, Y, Z, you need to call clearCache first". But that would be weird. I think, it better and reasonable to expect the owner of the polygon to not unnecessarily call isClockwise if the polygon wasn't changed. If you work with Clipper directly, you have to be careful not to call Area/Orientation unnecessarily too.

The user can call signedArea to determine the orientation of the polygon or whether it's an empty polygon if the area is 0. So we don't need isClockwise to return handle 0 area case by returning undefined. And I think isClockwise has been renamed now, so it should hold: p.isPositive === p.signedArea > 0. That doesn't mean that if it isn't positive that it is negative (a hole). Clipper doesn't return polygons with 0 area though, so in the case Clipper results, we know a polygon is a hole if it isn't positive.

@Feu-Secret Feu-Secret self-assigned this Feb 17, 2023
@Feu-Secret Feu-Secret changed the title PIXI.Polygon#isClockwise must not cache its result PIXI.Polygon#isClockwise Add the possibility to clear the cached result Feb 21, 2023
@dev7355608
Copy link
Author

I have to say it disagree with the choice to cache the orientation polygon for the reasons I explained in my previous comment. There wasn't even documentation added to isPositive regarding the caching. The documentation of clearCache doesn't explain who's responsibility it is to call it and when. Presumably it should be called immediately after the points of the polygon are manipulated, but how are you going to enforce that? You can't put restrictions on PIXI.Polygon like that. You have to create your own polygon class if you want to do caching.

Also this part of the issue was not addressed:

Also I don't think it makes sense to return undefined if there are less than 3 points. This is just one of many cases of degenerate polygons that do not have a well-defined orientation.

new PIXI.Polygon(0, 0, 0, 0).isClockwise // undefined
new PIXI.Polygon(0, 0, 0, 0, 0, 0).isClockwise // false

A polygon without well-defined orientation certainly doesn't have a clockwise orientation, so it makes sense to return false.

The documented return type still boolean. What's the benefit of returning undefined instead of false if the polygon has less that 3 points? If I want to know whether a polygon has less that 3 points, I can test that without the help of isPositive.

@Feu-Secret
Copy link

I think a ternary return is not meaningless. The polygon is either clockwise, counterclockwise, or degenerate (the undefined case). We should just ensure that all degenerate cases return undefined (if the signed area is 0)
Concerning the caching problematic, we'll see. Maybe we could create our own polygon class. For the moment, it is not a priority.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Issues related to the API used by Mod Devs bug Functionality which is not working as intended canvas Issues related to the PIXI canvas, rendering, and other WebGL functions.
Projects
Status: Done
Development

No branches or pull requests

4 participants