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

[lua] Add Global Variable details #5688

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

forbesmyester
Copy link

Fixes #5666

Add Lua API for getting details about Global Variables (not the value).

NOTE: I can and do code, but I really don't know C++, however I have put significant effort into getting this right. Thanks.

@philmoz
Copy link
Collaborator

philmoz commented Dec 1, 2024

Looks ok. Might want to remove the extra blank lines added before the #endif though.

@forbesmyester
Copy link
Author

Removed Lines.

@forbesmyester
Copy link
Author

I'm somewhat doubting this now. min/max are loaded & saved without issue, which suits my purpose (a load/save lua script)

However the numbers are the values stored presumably in memory, not the numbers the user sees on the screen, somewhat like what was happening in #4379

I'm looking back and following function calls through the handling of gvars on and end up with the following calls:

But that last function is somewhat of a monster and while I see it is largely that do..while loop that does the work, I'm not confident in editing those functions and feel I cannot copy it either as it's somewhat complex.

I feel either the implementation is fine, but the table values should be prefixed with "raw" or ask assistance on how to get the API values out as what the user sees on screen (which would be preferable).

Sorry I hoped to do this by myself :-(

@philmoz
Copy link
Collaborator

philmoz commented Dec 1, 2024

'unit' and 'prec' are not boolean values, they are integers (even though they are currently limited to be either 0 or 1).
I don't understand why you need to rename the 'min' and 'max' properties.

@forbesmyester
Copy link
Author

So, I am happy with the first 1st, 2nd or 3rd commits.

In the third commit I'm primarily looking at the user interface and noticing 2 things, as you say, unit and prec are integers, but displayed usage is as a boolean, so I thought that was better to have them as booleans. Happy to put them back to integers.

I also noticed that, getGlobalVariableDetails() the max value is represented as 124 when it would display 900 on screen (low precision). of course, using setGlobalVariableDetails() with a max of 124 will set it back to 900 (on screen). It's an integer in memory, but one that does not map directly to the screen / user perception... This is why I elected to rename it to raw_max, as it's not doing exactly what might be expected from a lua developer.

I'm working on an import/export script in lua and I don't really care, but am happy to go with either commit, or work with you or another team member to improve the API to be more obvious.

Let me know what you want to do.

Thanks

@philmoz
Copy link
Collaborator

philmoz commented Dec 1, 2024

'unit' and 'prec' are not treated as boolean in the UI - they select from a choice of values. If they were booleans they would be represented as toggle switches in the UI.

The internal values for 'min' and 'max' are offsets from GVAR_MIN and GVAR_MAX respectively - this is so that the default (0) value stored map to the default minimum and maximum. The stored value range is 0 - 2048 in both cases.

If you want to work with the -1024 - 1024 range in the Lua script then just apply GVAR_MIN and GVAR_MAX when getting and setting these values.
E.G.: to get the values:

      int16_t vmin = GVAR_MIN + g_model.gvars[gvar].min;
      int16_t vmax = GVAR_MAX - g_model.gvars[gvar].max;

@forbesmyester
Copy link
Author

Changed with your recommendations, thanks :-)

@forbesmyester
Copy link
Author

@philmoz is it right/correct for this to be requested to merge into main and how do we get that to happen? Thanks and Thanks for your help so far 🙂

@philmoz
Copy link
Collaborator

philmoz commented Dec 3, 2024

@philmoz is it right/correct for this to be requested to merge into main and how do we get that to happen? Thanks and Thanks for your help so far 🙂

@pfeerick will look after merging this when he has reviewed it (and has time).

@forbesmyester
Copy link
Author

Thanks and great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants