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

Make rectIntersection accept an array #6311

Merged
merged 8 commits into from Nov 23, 2016

Conversation

zhouyx
Copy link
Contributor

@zhouyx zhouyx commented Nov 22, 2016

close #6288

export function rectIntersection(a, b) {
const x0 = Math.max(a.left, b.left);
const x1 = Math.min(a.left + a.width, b.left + b.width);
export function rectIntersection(layoutRects) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make this a variadic function, instead of passing an array:

rectIntersection(rect1, rect2, rect3, rect4);

function rectIntersection() {
  // The "array" is `arguments`
  for (var i = 0; i < arguments.length; i++) {
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this. But what should I do with type check here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

const x1 = Math.min(a.left + a.width, b.left + b.width);
export function rectIntersection(layoutRects) {
const layoutRectArray = /** @type {Array<!LayoutRectDef>} */
(layoutRects.filter(rect => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use continue in the for loop instead of iterating and allocating another array.

}

current = layoutRectLtwh(x0, y0, x1 - x0, y1 - y0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can optimize this to only allocate 1 ever:

let x0 = first.left;
let x1 = x0 + first.width;
let y0 = first.top;
let y1 = y0 + first.height;

// Skip first
for (let i = 1; i < array.length; i++) {
  // Do maths on x0, x1, y0, y1
  // return null any time constraints are violated
}

return layoutRectLtwh(x0, y0, x1 - x0, y1 - y0);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

elementRect = this.element_.getLayoutBox();
dev().assert(elementRect.width >= 0 && elementRect.height >= 0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't necessary, not sure why we ever had it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deleted

let x1 = Infinity;
let y0 = -Infinity;
let y1 = Infinity;
let hasValidRect = false;;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this, just check if x1 is still Infinity.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sg

hasValidRect = true;
x0 = Math.max(x0, current.left);
x1 = Math.min(x1, current.left + current.width);
if (x1 < x0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe combine this with the if below? Saving two Math calls won't really change much.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -244,8 +244,6 @@ export class IntersectionObserverPolyfill {
// If opt_iframe is not provided, all LayoutRect has position relative to
// the host document.
elementRect = this.element_.getLayoutBox();
dev().assert(elementRect.width >= 0 && elementRect.height >= 0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other one too. 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😄 Didn't delete because the other one we accept elementRect as an input. Has no control of the input. However since the only usecase now lives inside custom-element.js I think it's fine to delete at this time.

Copy link
Contributor

@lannka lannka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job

@lannka lannka added the LGTM label Nov 23, 2016
@zhouyx zhouyx merged commit 24c36ba into ampproject:master Nov 23, 2016
@zhouyx zhouyx deleted the rectIntersection branch November 23, 2016 18:22
Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016
* git diff HEAD~1

* change intersection-observer cae

* take multi arguments

* remove console log

* ...

* minor

* small change

* remove another dev().assert
Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016
* git diff HEAD~1

* change intersection-observer cae

* take multi arguments

* remove console log

* ...

* minor

* small change

* remove another dev().assert
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Let rectIntersection support an array of LayoutRect
3 participants