feat: add scale_factor and extend value_factor (next)

[X-Ryl669]

See #877 for details.

Github doesn't allow to change the base branch onto which to PR once it's created, so this is taking over the previous PR that targeted master branch.

[akloeckner]

Taget dev branch...

I hope, I didn't send you on the wrong path. It just appeared to me (from comments in other PRs) that this project expects PRs against dev. New features seem to be there. In master, there's just updates in the README.

Btw, I noticed that the README change you have here is also on master, so maybe keep it out of this PR.

Using computeState...

Ok. I thought, value_factor is also applied only in computeState. So, this should work, I'd guess. And I'd judge it much cleaner. But if you tried and it didn't work, I withdraw my comment. :-)

Variable name scaleFactor...

Yes, I like that better, too. But I am no "competent authority", here. ;-) One thing to take into consideration, maybe, is that the two "factors" can now be easily mistaken for each other.

Just thinking out loud: Since the orignal idea was to introduce the new factor and deprecate the old factor... maybe it's good to just use the new factor in the actual computation. And convert the old factor to the new factor only once when reading the configuration. Maybe logging a deprecation notice at that time, too.

That would simplify things further. What do you think?

[X-Ryl669]

Ok. I thought, value_factor is also applied only in computeState. So, this should work, I'd guess. And I'd judge it much cleaner. But if you tried and it didn't work, I withdraw my comment. :-)

Yes. It's because value_factor is global to the card, so it can be applied globally, there's no need to pass it around (extrema, bounds, entities). But scaleFactor is per-entity specific, so I had to follow to which it applies to. Not the cleanest code, but it works without breaking too much of the existing logic.

Just thinking out loud: Since the orignal idea was to introduce the new factor and deprecate the old factor... maybe it's good to just use the new factor in the actual computation. And convert the old factor to the new factor only once when reading the configuration. Maybe logging a deprecation notice at that time, too.

I did this internally here (while instantiating the Graph object in main.js):

scaleFactor: (entity.scale_factor ? entity.scale_factor : 1)
            * (entity.value_factor ? 10 ** entity.value_factor : valueFactor),

Yes, I think it would be better to only have one factor. The value_factor should have been named value_power initially because, well, a factor is for a product and the actual operation is a power. But removing or renaming the existing variable would break compatibility, I don't think it's possible. I don't know how to deprecate a member either, so if some maintainer could express how to do that cleanly, I can change it. Maybe I can just remove it from the documentation?

I think it would be better if dev branch were updated on top of master so there isn't any possible confusion here.

[jlsjonas]

Welcome to the project @X-Ryl669 (& @akloeckner ) and thanks for your contribution!

As @akloeckner already mentioned, you indeed still have some commits from master on your branch (one of the reasons the github UI doesn't allow you to change target branches).

Could you rebase your branch on dev & drop the commits from master on your branch?
Ideally, master shouldn't be ahead, however @ildar170975 has contributed several documentation improvements on master since the last release

[jlsjonas]

As per #591 (comment), let's also make the code change cleaner by converting value_factor to value_multiplier on import (removing/replacing other occurrences) & error when both values are set in the config.

[X-Ryl669]

Renamed the variable.

[X-Ryl669]

I don't think it's an error when both are set in the config. At least, the current code supports both and they don't do the same thing.
The graph part already merges both parameters without any issue.
Anyway, I've rebased against dev now.

[andrewjswan]

Do you have an idea of the approximate time frame for implementing this change?

[akloeckner]

Not really. We were reluctant with new features at the time and still are in a way, because we have little time to debug and fix things, if they break.

My personal feeling with that specific PR is that it increases complexity by allowing a somehow redundant factor on two configuration levels, passing it around quite much. I'd feel much better, if we were to create a mechanism to do this consistently with all options, such as config.entities[i].show = {...config.show, ...config.entities[i].show}. But I haven't dug deep enough to be able to do this. Or even advise on how to do it.

[andrewjswan]

I don’t see the complexity here, value_factor in my opinion is more complicated and unclear, I would remove it altogether and leave only scale_factor, which can take both negative and positive values. And it would solve all the problems, and would completely replace value_factor and would be clearer.