Skip to content

Added movement and random block selection#5

Open
berdabo24 wants to merge 1 commit into
mainfrom
movement
Open

Added movement and random block selection#5
berdabo24 wants to merge 1 commit into
mainfrom
movement

Conversation

@berdabo24

Copy link
Copy Markdown
Collaborator

No description provided.

@invzfnc invzfnc left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Overall great work, just some things need tweaking. Details are in comments.

Comment thread app.js
//Get Random tetromino
let tArray = ["I","J","L","O","S","T","Z"];
let random = Math.floor(Math.random()*tArray.length);
let randtetromino = getTetrominoByName(tArray[random]);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Take a look at this: https://tetris.wiki/Random_Generator
Apparently you can't just randomly pick one from the seven tetrominos every single time you generate a tetromino. Instead you generate a sequence of 7 and pop out one at a time till the sequence is empty.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yep, noticed that. I was testing whether each block is able to function properly within the game boundaries. Will change it!

Comment thread app.js
}

//Get Random tetromino
let tArray = ["I","J","L","O","S","T","Z"];

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Rather than redefining write Object.keys(tetrominos) instead.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Roger

Comment thread app.js
let previousTimeStamp; // to compare with timeStamp
let y = 0; // position of falling tetromino
let x = 0;
let playfieldArray = Array.from({ length: gridRows }, () => Array(gridColumns).fill(0)); // converts playfield into 2D array

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Personally I think it would be better if we keep the naming consistent. Maybe better if we use playfieldMatrix like in getTetrominoByName. How you think?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yep I agree. Will make changes!

Comment thread app.js
Comment on lines +140 to +151
if (e.keyCode == 40){
if (isMoveValid(randtetromino, x, y) && y < 18) // <- some reason the tetromino overshoots, had to put another temp limiter here
y++;
}
if (e.keyCode == 37){
if (isMoveValid(randtetromino, x - 1, y))
x--;
}
if (e.keyCode == 39){
if (isMoveValid(randtetromino, x + 1, y))
x++;
}

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

https://stackoverflow.com/a/43418287
Use event.key == "ArrowDown" (etc) instead
This link might be useful https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key
And event instead of e is just my preference for clarity, it's up to you to choose which one.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Noted, will clarify it

Comment thread app.js
if (previousTimeStamp == undefined) { // first time calling
//Main gameloop
function gameloop(timeStamp) {
if (!previousTimeStamp) { // first time calling

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I like the touch up. Thanks.

@invzfnc invzfnc force-pushed the main branch 2 times, most recently from c283943 to 7fa30b4 Compare August 30, 2024 08:14
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.

2 participants