Closed Add00 closed 4 years ago
Hi Maybe we can see for merge your two extensions? With a proper general name? @4ian Noise and constant can be two keywords.
In any case the same reviews can be apply to this extension.
"Advanced mathematical", "Mathematical tools" (this one exist already but if you give the same name as the existing one in GD your functions and gdevelop function's will be in the same list.)
//Get function parameter
const playerId = eventsFunctionContext.getArgument("player_ID") - 1;
var newDeadzone = eventsFunctionContext.getArgument("deadzone");
if (playerId < 0 || playerId > 4) {
console.error('Parameter gamepad identifier in action: "Set gamepad deadzone for sticks", is not valid, must be between 0 and 4.');
return;
}
// clamp the newDeadzone in range [0, 1].
// https://github.com/4ian/GDevelop-extensions/pull/33#issuecomment-618224857
gdjs._extensionController.players[playerId].deadzone = gdjs.evtTools.common.clamp(newDeadzone, 0, 1);
So you can try something like:
gdjs._extensionNoise.qSeed = eventsFunctionContext.getArgument("seed");
Before you need to define your _extensionNoise in gdjs, look in Gamepad extension in the function onFirstSceneLoaded. It's basically a JS object ;)
For onFirstSceneLoaded, it's a lifecycle functions , there is not good resource about it on the wiki :/.
const x = eventsFunctionContext.getArgument("x"); const y = eventsFunctionContext.getArgument("y"); const z = eventsFunctionContext.getArgument("y");
Thanks for the explanation of lifecycle functions I never really understood them. I will change the seed function to the way you described and yes line 315 is wrong in perlin3 thanks for catching that :)
I'm fine having two separate extensions, as this one is really about noise functions while the other one is "just" mathematical constants.
Thanks for the feedback Bouh on my code I'm still pretty new at coding so your suggestions helped a lot (for both extensions). I added a onFirstSceneLoaded function and I fixed the mistake on line 315. I also agree with 4ian about keeping them separate as they both do two different things.
Let's go for a next review:
Remove unused/useless comment in JSEvents console.log() and unused code commmented. Keep the comments about how work the algorithm is fine 👍
The noise stuff are in the global scope.
This will be better if the code is define in the scope of the extension. You know in gdjs._extensionNoise
var module = gdjs._extensionNoise.noise = {};
There is also lot of duplication! I see grad() function in: perlin3, perlin2, simplex3, simplex2. This can be define once in onFirstSceneLoaded. It's hightly probable you need rework the structure. Here how is define one function in Gamepad extension:
gdjs._extensionController.axisToAngle = function (deltaX, deltaY) {
var rad = Math.atan2(deltaY, deltaX);
var deg = rad * (180 / Math.PI);
return deg;
}
I don't know in term of performance which choice is better, rework functions like Grad() in onFirstSceneLoaded and define the function once. Or, keep the current code but with duplication of these functions for perlin3, perlin2, simplex3 and simplex2.
Can you add caps on the first characters of each texts for each functions, in parameters too ;)
Finally can you check once the text and description, i see typo. dimnetional
Things done since last time:
I tried to move just the Grad function to the global scope, but it created too many issues so I just moved the whole noise library into the onFirstSceneLoaded and then only call the function needed in each expression
This is actually much better :) No duplication and no pollution of the global scope.
Code looks ok. I've done several changes in the descriptions and in the extension description and published it (commit d7d5a39cdcae064237741f27594ebdc6ae7445be). Thanks for working on this, will be very useful for procedural generation :)
⚠️ Please edit and complete this before submitting your extension:
Describe the extension
Adds 2D, 3D Perlin and simplex noise to GDevelop useful for procedural generation
Checklist
Extension file
noise.zip