bromagosa / Snap4Arduino

Binding Snap! and Arduino together
http://snap4arduino.rocks
GNU Affero General Public License v3.0
133 stars 85 forks source link

Wrong "Scale between ranges" block in library. #271

Closed DavidGasku closed 6 years ago

DavidGasku commented 6 years ago

The block "Scale from range () to range ()" is wrong, as you can see in the picture. graph calculator 4 script pic

The formula should be: graph calculator 4 script pic 1

If it is updated, I would recommend these two blocks:

Here you have the three blocks, in case you want to update the library:

scale blocks.zip

jguille2 commented 6 years ago

Hi @DavidGasku ,

Thanks for your report. Obviously, scale reporter is wrong, and we have to introduce the value-min correction. Maybe we can make constrained as an option. Then scale where constrain

We will have: scale1 and scale2

Note that constrain has two values (between 'a' and 'b') that are not 'min' and 'max' and 'scale' allows different gradients (positive or negative).

@bromagosa ? I will do this update.

DavidGasku commented 6 years ago

Great, that's better.

jguille2 commented 6 years ago

Hi, This fix it's easy. But we know libraries have a difficult maintenance (custom blocks definitions are stored inside the projects). So, to avoid further modifications, we must think better.

I saw the proposed block was too long and also, constrained boolean slot was not more elegant. So I propose a shorter block, using "over the range" for a non-constrained behavior and "into the range" for a constrained.

My proposal is two blocks (scale and constrain). Details:

Waiting for your opinions (@bromagosa, and also @DavidGasku )

Joan

bromagosa commented 6 years ago

I don't understand the "over" and "into" options, but otherwise constrain and scale look good to me.

jguille2 commented 6 years ago

Maybe are wrong expressions... Then I need your opinion.

The idea was:

"scale 6 from [0,5] over [5,10]" will report 11 (scaling without constraining) and

"scale 6 from [0,5] into [5,10]" will report 10 (scaling and constraining)

bromagosa commented 6 years ago

I see! I'm not sure "over" and "into" are the right words then. If I got it right, one of them (into) caps values outside the range to the range limit, and the other one (over) sort of applies "mod" to them, is that correct?

I can't think of a single use case for the second option, so I'd suggest always capping values into the range. A value that's outside the original range is an illegal value, so what to do with it is an arbitrary matter, isn't it?

"Give me the fourth element of this list of three elements" has multiple answers, but I'd expect the language to say "there's no fourth element" :)

jguille2 commented 6 years ago

To force constraining is not bad... but I think keeping this two options (constraining and no-constraining) is also good.

I note (I know this is the most important argument) that people coming from Arduino IDE, are using a map function (useful for example working with sensor values) and that map function is not constraining (the reference says "MAP does not constrain values to within the range, because out-of-range values are sometimes intended and useful. The constrain() function may be used either before or after this function, if limits to the ranges are desired.")

We can think about this. We must decide if we want one or two options and, if we choose two options, think about the best words.

We can use "scale value from [a,b] constrained/not constrained to [c,d]" (where constrained/not constrained are dropdown options). Maybe this is more clear that "into/over". But, if "into/over" was "sufficiently understandable", I think it would be a more beatiful option.

bromagosa commented 6 years ago

Okay, I see. I didn't understand the need to have this lib in the first place, since you can scale things yourself by using addition, subtraction, division and multiplication (should we add a "mean" block too? and a max block? and a min block? these are all things you're encouraged to write yourself), but since the only reason to have it was that people coming from the Arduino framework missed it, we should probably stick with what the Arduino libs do and have two blocks: scale (unconstrained) and constrain. Don't you think so?

jguille2 commented 6 years ago

I agree. So, two questions...

Finishing... @bromagosa make the decision... and I'll update the changes.

bromagosa commented 6 years ago

Let's keep the lib and fix it. Choose the version you like the most :)

jguille2 commented 6 years ago

Done!

Fixed with this commit. Finally I've chosen the shorter form. So, projects with "scale - from range - to range -" have the wrong (unfixed) block.

Thanks @DavidGasku for your report!

Joan