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

Restart the spiral iterator of the Multiply test when maxing out the grid size #53

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

shallawa
Copy link
Contributor

@shallawa shallawa commented Apr 26, 2024

#52

Refactor the spiral iterator into a separate class. Keep calling its next() method to get the coordinates ofthe next cell. When its isDone() returns true create a new iterator and make the tiles of upper layers have less lightness than the ones on the lower layers.

Add a new class for the roundedRect tile called "Tile". This class can handle the location, size and animation of the Tile.

@shallawa shallawa requested a review from smfr April 26, 2024 01:04
* THE POSSIBILITY OF SUCH DAMAGE.
*/

class SpiralIterator {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this justifies its own JS file; it seems specific enough to Multiply that you should just put it in multiply.js.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The class is moved to multiply.js.

@@ -320,6 +343,9 @@ Point = Utilities.createClass(
}
});

// FIXME: Add a seprate class for Size.
Copy link
Contributor

Choose a reason for hiding this comment

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

Given all the comments you had to add above, maybe just do this.

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 tried but the result was more involved. The problem are with add(), subtract() and multiply(). These methods expects Point, Size or just a Number. With Point act as Size, we have to deal with the Number as a special case.

So let's have this be fixed in a separate change.

@@ -24,120 +24,183 @@
*/
(function() {

class Tile {
Copy link
Contributor

Choose a reason for hiding this comment

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

The rest of the code doesn't yet use class syntax so I'm not sure this should yet.

Copy link
Contributor Author

@shallawa shallawa Apr 26, 2024

Choose a reason for hiding this comment

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

The ES6 class is removed and Utilities.createClass() is used instead. However this way is not great. Utilities.createClass() allows only one level of inheritance. I wanted to add a second level of inheritance between Stage and MultiplyStage

Stage

ReusableParticlesStage

MultiplyStage

I wanted to emphasize that the elements of Multiply are reusable. They are shown and hidden instead of created and removed. But I could not do that because Utilities.createClass() extends the prototype of the subclass with the prototype of the superclass only. It does not do that recursively. There might be tricky way to still do it. But I could not figure it out.

@@ -0,0 +1,968 @@
// Copyright 2014 Google Inc. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Unit tests are great but let's not add a bunch of testing code in a small incremental update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unit test is removed

@@ -0,0 +1,58 @@
<!--
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is necessary unless SpiralIterator gets used in more than Multiply.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The whole directory for unit test is removed.

let columnsCount = Math.floor(this.size.width / this.tileSize.width);
if (columnsCount % 2 == 0)
--columnsCount;
this.tileGrid = new Size(columnsCount, this.rowsCount);
Copy link
Contributor

Choose a reason for hiding this comment

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

When running this the visual jumps in the size of the grid feel like a bug. Is it possible to keep the visual rectangular size the same?

Copy link
Contributor Author

@shallawa shallawa Apr 26, 2024

Choose a reason for hiding this comment

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

Yes this can be done if the spiral iterator is restarted instead of having it enlarged. This will create layers of elements every time the spiral iterator is restarted. The opacity of the rounded rect tile can be a factor of its z-index and its distance from the center.

But this might be behavioral change and the score can be different. Are we okay with that?

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 implemented the restarting solution. And I made the rowsCount to be 59 always. Once the test maxes out the grid size it will create a new iterator.

@sky2 please try this solution. If you do not see the layers of elements on your machine change the rowsCount to be a smaller value.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like this one better in that the visual size of the grid doesn't jump around. thanks.

@shallawa shallawa force-pushed the eng/Fix-Multiply-maxing-out branch 2 times, most recently from c73e13c to 3151355 Compare April 26, 2024 18:22
…grid size

WebKit#52

Refactor the spiral iterator into a separate class. Keep calling its next() method
to get the coordinates ofthe next cell. When its isDone() returns true create a
new iterator and make the tiles of upper layers have less lightness than the ones
on the lower layers.

Add a new class for the roundedRect tile called "Tile". This class can handle the
location, size and animation of the Tile.
@shallawa shallawa force-pushed the eng/Fix-Multiply-maxing-out branch from 3151355 to 9307820 Compare April 26, 2024 23:50
@shallawa shallawa changed the title Expand the grid size of the Multiply test automatically when needed Restart the spiral iterator of the Multiply test when maxing out the grid size Apr 26, 2024
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.

3 participants