Code feedback on this custom envelope panel

[David Healey]

@dannytaurus

Looks very nice, can't you achieve the same thing with the flex ahdsr and some laf?

Here are my comments:

I'd draw the labels on the panel rather than using individual label components.

@dannytaurus said in Code feedback on this custom envelope panel:

function clamp(x, a, b) { if (x < a) return a else if (x > b) return b else return x; }

Why a function and not an inline function? Doesn't Math.range do the same thing?

@dannytaurus said in Code feedback on this custom envelope panel:

if (n >= 0.5)
return -12.0 + (n - 0.5) / 0.5 * 12.0;
else
return SUSTAIN_DB_MIN + (n / 0.5) * (-12.0 - SUSTAIN_DB_MIN);

You don't need the else in statements like this. The first return will exit the function anyway.

@dannytaurus said in Code feedback on this custom envelope panel:

if (d >= -12.0)
local n = 0.5 + (d + 12.0) / 12.0 * 0.5;
else
local n = (d - SUSTAIN_DB_MIN) / (-12.0 - SUSTAIN_DB_MIN) * 0.5;

I think it's cleaner to declare n outside of the if statement rather than have two declarations for it (I know only one will actually be hit but it has more potential for error if you expand the function in the future).

@dannytaurus said in Code feedback on this custom envelope panel:

// Draw handles
inline function drawBall(key, x, y)
{

I didn't know you could declare a function within a paint routine. Somehow that feels wrong to me :) I'd declare it outside, but if it works it works.

@dannytaurus said in Code feedback on this custom envelope panel:

  var w = this.data.drag.which;

  if (w == "A")

This is a perfect place for a switch statement instead of the else ifs

[dannytaurus]

@Christoph-Hart I'll take a look at Flex AHDSR.

I just thought this would be a good time to investigate how to make custom panels. A lot of the stuff I want to do in the future will need custom controls, so I wanted to learn the ins and outs.

[dannytaurus]

@David-Healey said in Code feedback on this custom envelope panel:

I'd draw the labels on the panel rather than using individual label components.

The labels aren't set in stone yet. I tihnk eventually I'd like the user to be able to drag the label values up/down to change the values too, as well as the handles.

Why a function and not an inline function? Doesn't Math.range do the same thing?

I was getting some errors calling it when it was an inline function. I'll look at Math.range too

You don't need the else in statements like this. The first return will exit the function anyway.

Yep, good point

I think it's cleaner to declare n outside of the if statement rather than have two declarations for it

That's just me squeezing a couple of line out of the code, but yeah probably best to define outside

I didn't know you could declare a function within a paint routine. Somehow that feels wrong to me :) I'd declare it outside, but if it works it works.

Yep, new to me too but it works. But yeah, might move it outside for clarity. Especially since it's multiple lines. It was a one-liner until I added the handle highlight colour.

This is a perfect place for a switch statement instead of the else ifs

I read you shouldn't declare vars in a switch statement, so I stuck with if/else. I can move the x and n vars outside I guess

Thanks for all the tips!

[Oli Ullmann]

@dannytaurus
That looks great!

@Christoph-Hart I also created my own panel because I had performance issues with theFlexEnvelope FloatingTile. It caused significant lag in the user interface and the interface of my DAW (Cubase).

[ustk]

@David-Healey said in Code feedback on this custom envelope panel:

I didn't know you could declare a function within a paint routine. Somehow that feels wrong to me :) I'd declare it outside, but if it works it works.

I tend to do it this way nowadays, inlining as many functions as I can for paintRoutines, mouseCB... And sub functions can be used elsewhere when necessary

MainPanel.setPaintRoutine(drawMainPanel);

inline function drawMainPanel(g)
{
	subFunction1();
	subFunction2();
}


inline function subFunction1()
{
	
}

inline function subFunction2()
{
	
}

[David Healey]

@ustk Yes that's my approach too.

A few things in my mind when writing code:

• Don't indent more than 2 levels - there are exceptions, for example when using nested loops.
• Make the code readable, even if it ends up a little more verbose.
• The code should be self documenting as much as possible, no need for comments
• Be consistent with naming, use of brackets, etc.

Basically the stuff I put in the style guide.

I also use type-safety for all my functions now, except in a few instances where I want to allow for undefined return types.

[ustk]

@David-Healey all the same except for type-safety which I didn't really see the gain for me now as I am very careful on the caller side... But I understand where it can help

[dannytaurus]

@ustk said in Code feedback on this custom envelope panel:

I tend to do it this way nowadays, inlining as many functions as I can for paintRoutines, mouseCB...

Is this mainly for readability and reusability? Or is there a performance improvement too?

[ustk]

@dannytaurus inline functions tend to perform better

[dannytaurus]

@ustk I thought inline functions were placed inline at their call spot during compilation, hence the name? Which would end up putting them all back where they were refactored out from.

Although I now think I remember Christoph saying recently that wasn't the case?

[David Healey]

@ustk said in Code feedback on this custom envelope panel:

@dannytaurus inline functions tend to perform better

For paint routines and mouse callbacks I'm not sure it makes a difference since the paint routine or mouse callback is still a regular function.

@dannytaurus said in Code feedback on this custom envelope panel:

Although I now think I remember Christoph saying recently that wasn't the case?

You remember correctly, he was lying to us this whole time :p

[dannytaurus]

@David-Healey @ustk I'm not against the concept of refactoring stuff out to inline functions at all, it's a very common pattern in Ruby. Lots of small well-named methods can make the code a lot easier to reason about.

Also helps with the self-documenting code idea.

@David-Healey

[ustk]

@David-Healey said in Code feedback on this custom envelope panel:

For paint routines and mouse callbacks I'm not sure it makes a difference since the paint routine or mouse callback is still a regular function.

Yes but the way I understand it (might be wrong though) is that it's faster to execute even if the outside part isn't inlined. The inline part containing locals, the code execution inside it seems faster especially for long paint routines or mouseCB. Then what happens after this isn't affected but at least the script part might be improved.