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

Overhaul #9

Merged
merged 100 commits into from
Dec 18, 2022
Merged

Overhaul #9

merged 100 commits into from
Dec 18, 2022

Conversation

Rudxain
Copy link
Contributor

@Rudxain Rudxain commented Jun 19, 2022

  • Fixed inconsistent whitespace and indentation
  • Added ESLint
  • Refactoring
  • Edited and added a few comments, also JSdoc type annotations
  • Added "placeholder hint" for new users
  • Made OOP class to abstract command history operations, including a feature that fixes "user-generated memory-leaks"
  • Added automatic 🌙dark-theme support
  • Using Sans-Serif font instead of default
  • Declared all variables using let & const
  • Converted fns to arrow syntax
  • Replaced all querySelector calls by getElementById. This is better for readability (I guess?). However, performance is more unpredictable (it depends on some factors), and there's an important difference detail between both
  • Fixes Command doesn't run if it's repeated #10
  • Made UI more accessible to mobile users

@GoToLoop
Copy link

GoToLoop commented Jun 19, 2022

Declared all local variables using let.

When we know a variable isn't supposed to be re-assigned we should declare it w/ const instead:
https://developer.Mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/const

var drops = Array(n);

The constructor Array() creates a sparse array which is slower than a regular dense 1:
https://developer.Mozilla.org/en-US/docs/Web/JavaScript/Guide/Indexed_collections#sparse_arrays

There's a belief that invoking method fill() might turn a sparse array into a dense 1:
const drops = Array(n).fill();
https://developer.Mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/fill

@Rudxain
Copy link
Contributor Author

Rudxain commented Jun 19, 2022

We should declare it w/ const instead

That's exactly what I was thinking, but I didn't want to be "intrusive". I'll replace to const when possible. Also, should the examples use let & const too? Or should I leave them intact with var?

The constructor Array() creates a sparse array which is slower than a regular dense.

That's true, but it doesn't matter if new is used or not, the result is exactly the same.

There's a belief that invoking method fill() might turn a sparse array into a dense

That's not a valid belief, because it's actually a fact. Array(len).fill(0) always returns a dense array (only from the POV of the caller). However, the internal implementation is another story: The array might still be sparse, until the engine realizes it's actually dense. Or, depending on engine, the array might get "stuck" as sparse for the entire runtime, until it gets replaced by a dense array object. I was thinking of using TypedArrays, but they have compatibility issues. A better solution is to manually create an empty array using the literal [], and then push values until it has the desired size

@Rudxain
Copy link
Contributor Author

Rudxain commented Jun 19, 2022

@GoToLoop Done. I edited the main comment to reflect the changes

@Rudxain
Copy link
Contributor Author

Rudxain commented Jun 19, 2022

Lol, I realized the Array(n) was totally unnecessary, I replaced it by []

@GoToLoop
Copy link

GoToLoop commented Jun 20, 2022

Or should I leave them intact with var?

If I can't change var to const I don't bother to change it to let. 😛

, but it doesn't matter if new is used or not, the result is exactly the same.

Yea, that Array(arrayLength) constructor call signature is sparse w/ or w/o keyword new: 😞
https://developer.Mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/Array

The array might still be sparse, until the engine realizes it's actually dense.

My belief is that by now all modern JS engines should be smart enough to internally convert a sparse array to a dense 1 after a fill() method call. 🙏
const drops = Array(n).fill();

I was thinking of using TypedArrays, but they have compatibility issues.

Well, drops[] is an array of objects; therefore it can't be a typed array regardless. 🤷‍♂️

... create an empty array using the literal [], and then push() values until it has the desired size.

You've done that now. However, it's still adding objects using the [] notation instead of push()! 🤔

This is my push() take on init_drops(): 💧

function init_drops(n) {
   const drops = [];

   n = ~~Math.abs(n);
   
   while (n--)  drops.push({
      x: random(-150, 150),
      y: random(-150, 150),

      velocityX: random(-6, 6),
      velocityY: random(-6, 6),

      size: random(20, 300),
      width: random(1, 40),

      r: random(0, 255),
      g: random(0, 255),
      b: random(0, 255),
      a: Math.random()
   });

   return drops;
}

@Rudxain
Copy link
Contributor Author

Rudxain commented Jun 20, 2022

My belief is that by now all modern JS engines should be smart enough to internally convert a sparse array to a dense 1 after a fill() method call. 🙏 const drops = Array(n).fill();

I agree and I also hope that. There seems to be a lot of push (pun intended lol) for functional & declarative programming constructs optimization

Well, drops[] is an array of objects; therefore it can't be a typed array regardless. 🤷‍♂️

I focused so much on turtle.js that I completely overlooked that fact lmao. Thank you for making me notice.

... create an empty array using the literal [], and then push() values until it has the desired size.

You've done that now. However, it's still adding objects using the [] notation instead of push()! 🤔

True lol. I saw the i and the thought of using push didn't cross my mind, or maybe it did (subconsciously) but I was too lazy to realize and accept it 🤔

About your code: it looks so clean! I think I'll add it now

From 2^16 to 2^20 CUs, approximately ~128KB to ~1MB
Code by @GoToLoop
[Link to Issue comment that contains the code](#9 (comment))
@Rudxain
Copy link
Contributor Author

Rudxain commented Jun 20, 2022

I'm still kinda hesitant of refactoring all examples to use let & const. I'll wait for approval by the owner

The `if` has been turned into a `while` to handle the edge case that multiple cmds may need to be removed from history
RIP `var` lol. I thought IE didn't support `const` within loops, because MDN said it didn't support `let` in loops
@Rudxain
Copy link
Contributor Author

Rudxain commented Jun 20, 2022

I think there should be a feature to allow users to manually force the code to remove old history cmds. This is good because it improves performance by avoiding automatic garbage collection, and users would no longer need to reload the page to lose their entire history.

A related feature would be to allow direct access to certain history entries, instead of sequential access. It seems simple to implement, just add a button that "teleports" back a number of entries specified in an input text-box.

I'll try implementing both later (maybe some hours or days), when I have enough time.

EDIT:
I realized those 2 features may be useless. Users shouldn't be given the cognitive load of manually deciding when to clear a slice of the history and how many entries to remove. That just adds unnecessary complexity to the site, and redirects the focus of some users to the wrong place. I haven't tested the code, but the hi-level GC should be fast enough that nobody would notice it exists (only if the browser has a dual-ended array to optimize shift to be as fast as pop)

Direct access is a feature that not even browser dev consoles, nor linux terminals have, and nobody has complained. I don't see any reason to add it

@bjpop
Copy link
Owner

bjpop commented Jun 20, 2022

Hi @Rudxain and @GoToLoop. Thank you for working on this code. I see a lot of activity here. Should I start looking at the pull request or should I wait until you've had more time?

@Rudxain
Copy link
Contributor Author

Rudxain commented Jun 20, 2022

Thank you for working on this code. I see a lot of activity here. Should I start looking at the pull request or should I wait until you've had more time?

You're welcome. I think I might need some more time to re-review some stuff. And I have a question about the examples directory: should all declarations use var? Or is it okay to replace by let & const (where possible)?

I replaced those declarations only in the main file because I saw a few let used there, but I didn't saw any let used in any of the examples, so I leave it untouched just-in-case

There was a tab instead of spaces
This isn't a terminal, but it kinda behaves like it is
@Rudxain Rudxain changed the title Some edits A LOT of edits Jun 20, 2022
@Rudxain
Copy link
Contributor Author

Rudxain commented Jun 20, 2022

@bjpop the PR is ready. I don't know if @GoToLoop has something to say.

I kept using var for the examples, JIC. I also found that spiral accidentally declared globals because it assigned directly

@Rudxain
Copy link
Contributor Author

Rudxain commented Jun 20, 2022

Wait! I want to try to implement text rotation in write, I don't know if I can, but maybe it can happen.

I don't know if that should be a separate PR, or if I should do it in this PR.

Update: I gave up lol

examples/sierpinski.js Outdated Show resolved Hide resolved
@Rudxain
Copy link
Contributor Author

Rudxain commented Nov 24, 2022

I guess this is ready now 🤷‍♂️. I'm awaiting feedback

@Rudxain Rudxain marked this pull request as ready for review November 24, 2022 19:01
@bjpop
Copy link
Owner

bjpop commented Dec 3, 2022

Hi again Hi @Rudxain and @GoToLoop.

Is this PR ready for me to merge?

Is there a summary of all the main things that have been done?

Thanks heaps for all your work on this.

@Rudxain
Copy link
Contributor Author

Rudxain commented Dec 3, 2022

Hi

Is there a summary of all the main things that have been done?

I decided to edit the "main" comment everytime a "major" commit updates something. So everything should be there, unless I forgot something

Thanks heaps for all your work on this.

You're welcome!

@bjpop bjpop merged commit 55e6124 into bjpop:master Dec 18, 2022
@bjpop
Copy link
Owner

bjpop commented Dec 18, 2022

Thanks heaps @Rudxain and @GoToLoop for your work on this!

@Rudxain Rudxain deleted the patch-1 branch December 18, 2022 09:56
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.

Command doesn't run if it's repeated
3 participants