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

Why not use more helper functions to eliminate repetition? #68

Closed
RandomGamingDev opened this issue Sep 6, 2024 · 6 comments
Closed

Comments

@RandomGamingDev
Copy link

Repetition of functions that don't need that repetition seems to be common to the point where it's extreme
One example are the trigonmetric functions, for example:

	$.sin = (a) => {
		if ($._angleMode == 'degrees') a = $.radians(a);
		return Math.sin(a);
	};
	$.cos = (a) => {
		if ($._angleMode == 'degrees') a = $.radians(a);
		return Math.cos(a);
	};
	$.tan = (a) => {
		if ($._angleMode == 'degrees') a = $.radians(a);
		return Math.tan(a);
	};
	$.asin = (x) => {
		let a = Math.asin(x);
		if ($._angleMode == 'degrees') a = $.degrees(a);
		return a;
	};
	$.acos = (x) => {
		let a = Math.acos(x);
		if ($._angleMode == 'degrees') a = $.degrees(a);
		return a;
	};
	$.atan = (x) => {
		let a = Math.atan(x);
		if ($._angleMode == 'degrees') a = $.degrees(a);
		return a;
	};
	$.atan2 = (y, x) => {
		let a = Math.atan2(y, x);
		if ($._angleMode == 'degrees') a = $.degrees(a);
		return a;
	};

in q5-math.js,

			dx *= $._da;
			dy *= $._da;
			dw *= $._da;
			dh *= $._da;
			sx *= $._da;
			sy *= $._da;
			sw *= $._da;
			sh *= $._da;

in q5-2d-image.js,

and to a lesser extent

		$._filters[Q5.THRESHOLD] = (data, thresh) => {
			if (thresh === undefined) thresh = 127.5;
			else thresh *= 255;
			for (let i = 0; i < data.length; i += 4) {
				const gray = 0.2126 * data[i] + 0.7152 * data[i + 1] + 0.0722 * data[i + 2];
				data[i] = data[i + 1] = data[i + 2] = gray >= thresh ? 255 : 0;
			}
		};
		$._filters[Q5.GRAY] = (data) => {
			for (let i = 0; i < data.length; i += 4) {
				const gray = 0.2126 * data[i] + 0.7152 * data[i + 1] + 0.0722 * data[i + 2];
				data[i] = data[i + 1] = data[i + 2] = gray;
			}
		};
		$._filters[Q5.OPAQUE] = (data) => {
			for (let i = 0; i < data.length; i += 4) {
				data[i + 3] = 255;
			}
		};
		$._filters[Q5.INVERT] = (data) => {
			for (let i = 0; i < data.length; i += 4) {
				data[i] = 255 - data[i];
				data[i + 1] = 255 - data[i + 1];
				data[i + 2] = 255 - data[i + 2];
			}
		};
		$._filters[Q5.POSTERIZE] = (data, lvl = 4) => {
			let lvl1 = lvl - 1;
			for (let i = 0; i < data.length; i += 4) {
				data[i] = (((data[i] * lvl) >> 8) * 255) / lvl1;
				data[i + 1] = (((data[i + 1] * lvl) >> 8) * 255) / lvl1;
				data[i + 2] = (((data[i + 2] * lvl) >> 8) * 255) / lvl1;
			}
		};

in q5-2d-soft-filters.js

These contain a lot of repetition that could otherwise be reduced to a significantly smaller scale quite easily using vanilla JS and currently take up far too much effort and are too tedious to change, and in general fill up the screen which has many negative ramifications.

While I would understand if this was chosen for performance reasons, this is one of the exact reasons, and one of the largest reasons that p5.js's codebase has become so janky, weighted, and hard to maintain (e.g. p5.js's own p5.Math math library), which is exactly what q5.js is trying to avoid, and is an even bigger issue for q5.js due to its reduced backing compared to p5.js, which has a massive global and corporational backing.

If performance isn't a big issue, I'd recommend using Vanilla JS which has minimal performance impacts, especially with modern JS engines, but if it is, I'd recommend using a Javascript preprocessor or macro library (whichever you choose is up to your goals).

Once a decision is made on whether to remove the repetition or not and how to do so, I'd be happy to create some PRs to help reduce it.

@RandomGamingDev RandomGamingDev changed the title Fix repitition of identical functions Fix code quality deterioration from repetition of code Sep 6, 2024
@quinton-ashley
Copy link
Collaborator

While creating a function to encapsulate repetitive code is often a best practice, running functions in JS does have a performance cost. Sometimes it's worth trading nicer looking code for better performance.

Also, q5 is made with "vanilla JS", which is a term used to describe plain JS code that doesn't use any libraries and doesn't need to be compiled.

@RandomGamingDev
Copy link
Author

While creating a function to encapsulate repetitive code is often a best practice, running functions in JS does have a performance cost. Sometimes it's worth trading nicer looking code for better performance.

Also, q5 is made with "vanilla JS", which is a term used to describe plain JS code that doesn't use any libraries and doesn't need to be compiled.

I'm guessing that macros are out of the pictures then. Do you think that reducing repetition via methods in Vanilla JS is the way to go and something worth investing effort into through PRs though?

@quinton-ashley
Copy link
Collaborator

quinton-ashley commented Sep 6, 2024

The trigonometric functions can be used hundreds of thousands of times per frame in some sketches, so having an if statement instead of a function call can make a significant difference.

Try editing the q5 code as you see fit and run performance tests comparing it and you can see what I mean.

Though, you have given me an idea on how to reduce the repetition in the trig functions code: by having loops define the functions. I will test that out soon.

@quinton-ashley quinton-ashley changed the title Fix code quality deterioration from repetition of code Why does q5 sometimes use repition instead of encapsulation? Sep 6, 2024
@quinton-ashley quinton-ashley changed the title Why does q5 sometimes use repition instead of encapsulation? Why does q5 sometimes repeat lines of code instead of using helper functions? Sep 6, 2024
@quinton-ashley quinton-ashley changed the title Why does q5 sometimes repeat lines of code instead of using helper functions? Why not use more helper functions to eliminate repition? Sep 6, 2024
@quinton-ashley quinton-ashley changed the title Why not use more helper functions to eliminate repition? Why not use more helper functions to eliminate repetition? Sep 6, 2024
@quinton-ashley
Copy link
Collaborator

@RandomGamingDev I added the loops for defining the trig functions in v2.2
https://github.com/q5js/q5.js/blob/main/src/q5-math.js#L52

I forgot to respond to your question about PRs. I'll certainly accept PRs that improve q5's code quality as long as performance is not degraded.

@quinton-ashley
Copy link
Collaborator

I'm going to revert the previous change to the trig functions because doing it the way I did in v2.2 makes them unnamed in performance logs, displaying as $.<computed>. Also I figured out how to make the trig functions shorter and faster, will update soon!

@quinton-ashley
Copy link
Collaborator

updated trig functions in v2.3

$.sin = (a) => Math.sin(!angleMode ? a : a * DEGTORAD);

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

No branches or pull requests

2 participants