JdeRobot / VisualCircuit

Visual programming of robots using hardware blocks composition and Python
https://jderobot.github.io/VisualCircuit/
13 stars 14 forks source link

Added unit conversion feature in constants block and internal code frequency input #272

Closed BkPankaj closed 8 months ago

BkPankaj commented 8 months ago

Changes Made

  1. Implemented the addition of three units: k or K, m, M (representing Thousands, milli, Mega) to facilitate seamless conversion for both constant input and internal code block.
  2. Also, added the internal code block frequency section to accept units. If the value is a decimal and close to 0, it will be set to 0 (e.g., 4m).
  3. In the Cropper and various other blocks, multiple parameters are included within a single constant. To address this, each value is now separated by commas. Subsequently, the necessary conversions are applied, followed by the reintroduction of commas. This ensures the internal code functions smoothly without encountering any issues.

Related Issues

Fixes: #271

Screenshots

(Used all values for example purpose)

Canvas area - Cropper internal block with xywh of 5.73k,0.0012m,12M,248 and frequency 2.46M

data.json- with xywh of 5.73k,0.0012m,12M,248 (5730, 0.0000012, 12000000,248) and frequency 2.46M ( 2460000)

Canvas area - Threshold internal block with low of 5.0832K and high of 18m and frequency 3m

data.json- with low of 5.0832K (5083.2) and high of 18m (0.0180000) and frequency 3m i.e., 0.003 (0)

toshan-luktuke commented 8 months ago

Hi Pankaj, thank you for the contribution. The PR looks quite good... a question though

  1. At what point does the unit conversion take place in the Canvas? From my understanding it seems to be after the constant widget detects a change, if so does it change the value displayed in the Canvas or not?

In addition it is not desired to set low values equal to 0, as a user may purposefully use such low units in their program.

BkPankaj commented 8 months ago

Hello @toshan-luktuke , Yes, you are correct. Whenever it detects a change, it converts internally. I find this to be the best solution for real-time conversion. Basically, whenever the component called again (screen change) then it display converted value, the main aim is to avoid displaying the converted value on the spot. This is because it may make it difficult for the user to understand what happened. For example, if "27.98M" is displayed as "27980000" immediately, it could confuse the user. Therefore, after the screen changes, the converted value is shown. In other terms can say, If it is an internal block, and you visit it again internally, you will find the converted value. When you use it on the main canvas, it will show the value with the unit until the person enters the internal block, and then comes back to the main canvas. You will find the converted value then.

Regarding setting a low value to 0 for the internal code block, for that, I thought setting the frequency to 0.25 is close to 0, so I did that. No issue, I will change that to a decimal.

BkPankaj commented 8 months ago

@toshan-luktuke kindly check, I have changed parseInt to parseFloat for accepting decimal values inside the code block in the 'frequency' section. Thank you for the suggestion.

toshan-luktuke commented 8 months ago

Alright, looks good, could you write comments for what the regex statements are doing once? I think we can merge after that

BkPankaj commented 8 months ago

Hello @toshan-luktuke, could you please review my comments for the unit conversion function, handle change function, and regex statements? Thank you!

toshan-luktuke commented 8 months ago

Do check my review once @BkPankaj

BkPankaj commented 8 months ago

@toshan-luktuke where you inserted review? Let me know which changes required. I have already committed changes as per you said in above comment.

toshan-luktuke commented 8 months ago

You can go up where I've highlighted the code, or look in the Files Changed tab and you should see it

BkPankaj commented 8 months ago

@toshan-luktuke I still cannot find your highlighted code or in the file changes. I don't know what I am missing. Could you please share a picture showing where it is or how to access it? It would be helpful for me as I have tried everything and am not able to find it.

toshan-luktuke commented 8 months ago

image Its just this comment... Alternatively, in the Files Changed Tab: image

BkPankaj commented 8 months ago

Thank you for bringing it to my attention, @toshan-luktuke. I still couldn't locate your comment or review in the 'Files changed' section. Nevertheless, based on the images you provided, I acknowledge that I missed removing that line. Initially, I was considering functionality where, for example, if a user entered '10,000,' it would be handled. However, I later realized there might be several constants with more commas, so I abandoned that idea. Unfortunately, I forgot to delete that line. Additionally, in the comments commit, I only added a comment for that regex statement not seen use since the output was correct. In the new commit, I have rectified these issues. I have also added support for negative numbers, which was missing in the initial commits. Now, I believe there shouldn't be any issues.

toshan-luktuke commented 8 months ago

Looks good now, thanks a bunch for your contribution @BkPankaj !

BkPankaj commented 8 months ago

Thank you for merging @toshan-luktuke ! I'm excited to see this feature go live. Today marks my first contribution to a large codebase and project, and I'm very happy about it. If you have any feedback or suggestions, please feel free to let me know. Looking forward to more collaboration and making further contributions!