Adjusted rpm and deg/s to rad/s by AlexG1031 · Pull Request #212 · PMKS-Web/PMKSWeb

AlexG1031 · 2024-02-12T18:05:02Z

This PR resolves multiple issues:

The input speed (in yellow) should always show rpm even if another unit is entered (used to have no units). This box should not allow negative numbers.
The angular velocity (in red) should changed between deg/s and rad/s based on the angle unit set on the setting panel.
Changing input direction should change the analysis graphs. For example angular velocity of the crank should be positive when CCW.
Fix error messages caused by graph subscriptions that were not properly unsubscribed.

netlify · 2024-02-12T18:09:39Z

✅ Deploy Preview for pmksprod ready!

Name	Link
🔨 Latest commit	`53a7b2f`
🔍 Latest deploy log	https://app.netlify.com/sites/pmksprod/deploys/65dce5ab78736d0008af899e
😎 Deploy Preview	https://deploy-preview-212--pmksprod.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

… for input speed.

KohmeiK

Reviewing: https://deploy-preview-212--pmksprod.netlify.app

Great work on creating the new rotational unit, that all looks good! You're missing a couple parts to make this fully work though. If you look a the other input fields, when you type in just a number, the unit autofills. (So entering 20 would give 20 rpm). Also, typing in 20 rpm now makes the analysis label got to NaN.

A couple things to look at on how to fix this.

Take a look at parseLengthString in number-unit-parser.service.ts to see how to handle text inputs to find the right units. You need something similar for the rotational units.
Take a look at this.jointForm.controls['yPos'].valueChanges.subscribe (Edit panel line 216) for how to handle a input field, it should be very similar

Also the graph should update when you change the input speed right? Right now, even though it shows RPM I think it's still using some other unit in the simulator.

Take a look at Line 163 in the setting panel for how to do this. this.mechanismSrv.onMechUpdateState.next(2);

KohmeiK · 2024-02-19T16:38:27Z

src/app/component/settings-panel/settings-panel.component.ts

      this.settingsForm.patchValue(
        { objectScale: this.currentObjectScaleSetting.toString() },
-        { emitEvent: false }
+        { emitEvent: false },


This looks like we are resetting the input speed when the object scale is changed? Do we want to reset the input speed back to 20?

We do not need to reset the input speed back to 20. I can make sure that when we change the grid, the inputVal stays the same value as assigned.

@KohmeiK I have a question on this part. For this objectScale, shouldn't we get rid of this emitEvent? I imagine we want to update the graph, right?

A bunch of these emitEvent: false is true so that we don't cause a circular loop and therefore a stack overflow. Updating the value will cause the onChange to emit, which will cause the value to change and the onChange to emit and so on.

Makes sense!

KohmeiK · 2024-02-19T16:46:07Z

src/app/component/settings-panel/settings-panel.component.ts

    this.settingsForm.controls['speed'].valueChanges.subscribe((val) => {
      if (this.settingsForm.controls['speed'].invalid) {
-        this.settingsForm.patchValue({ speed: this.currentSpeedSetting.toString() });
+        this.settingsForm.patchValue({ speed: this.nup.formatValueAndUnit(Number(val), this.settingsService.rotationalUnit.getValue()),


This will set the unit to always we be the setting's rotational unit value, ignoring the user's unit input. So 20 rps will be 20 rpm. Take a look at edit pane line 218 for a better example of how to handle this:

const [success, value] = this.nup.parseLengthString( val!, this.settingsService.lengthUnit.getValue() );

You can see that the function takes the big string (with the value and unit) and converts it into a value with the units we want.

This is a good catch!

I'm creating another function called parseAngVelRateString that does a functionality similar to parseLengthString.

KohmeiK · 2024-02-19T16:48:08Z

src/app/component/settings-panel/settings-panel.component.ts

  }

-  numRegex = '^-?[0-9]+(.[0-9]{0,10})?$';
+  numRegex = '^-?[0-9]+(\\.[0-9]{0,10})?\\s*[a-zA-Z]*$';


I don't think we should use the regex for the input field with a value and unit. Instead the current behavior for the other combo input fields (where it's not just a number) is to revert back to the original number. (Try this for the edit panel input fields)

KohmeiK · 2024-02-19T16:48:47Z

src/app/services/number-unit-parser.service.ts

        return value.toFixed(2) + ' N';
+      case RotationUnit.RPM:
+        return value.toFixed(2) + ' rpm';
+      case RotationUnit.RPS:


This should be radians / second right? Not revolutions per sec

…rther testing.

KohmeiK · 2024-02-22T04:41:26Z

The text box looks really good! One small issue I see is when you type a invalid value in, it results in the original number populated but the unit is gone. Also, any rounding is gone. Try '1dps' then 'random'. It first correctly shows 0.17 rpm but reverts to 1.6666 after

AlexG1031 · 2024-02-23T14:03:50Z

I corrected the error regarding showcasing the unit if someone types something incorrectly.

I am not 100% sure what you mean by 'reverts to 1.666 after'. From playing around with this, I think this acts how I would expect this to act.

Also, I am not sure what the error is regarding rounding. From my observation, the rounding appears to happen properly.

Do let me know if this fixes all the issues that you had found on your side.

…types in a unit incorrectly

KohmeiK · 2024-02-23T15:18:19Z

Looks great! I don't want to add more than the original spec but I noticed that the clockwise / counter clockwise doesn't switch the input speed

…xtbox

…ngAcc

KohmeiK · 2024-02-26T19:51:57Z

Super close to being done! Can you make a couple minor changes?

When a negative number is entered into the input box, change the value to the positive number and flip the input direction automatically. So if you have it as CCW and the user puts in -30 rpm, set the input speed to 30 rpm and change it to CW. The error message should be more clear and user-friendly. Something like: "The input speed must be positive. The input direction has been reversed."
I still see the subscriber errors by loading a mechanism, opening and closing a graph, then changing the angle unit or the global (length units). I think the solution you suggested will work to fix this.
Can you double check the data correctness? For example if I put in 20 rpm as the input speed, and look at the graph for the speed of the crank, I see it shown as 20 deg/sec. Instead it should be 1200 deg/sec right? It might be just be a scaling problem but can you find a way to use Jessica's Matlab script (or the old PMKS) to verify that a couple of graphs are at least in the right ballpark number? I don't want this PR to become the verification task all over again but we just need to make sure the value is no egregiously wrong

Adjusted rpm and deg/s to rad/s

878848c

AlexG1031 requested a review from KohmeiK February 12, 2024 18:05

Settings panel now showcases rotational unit (rpm) within the textbox…

21a717d

… for input speed.

KohmeiK requested changes Feb 19, 2024

View reviewed changes

AlexG1031 added 2 commits February 19, 2024 15:32

Update most comments based from what Kohmei mentioned. Still needs fu…

1b5faf6

…rther testing.

Believe textbox within settings works properly

7caca22

Fixed the bug to showcase unit if the user does not insert a unit or …

70aba92

…types in a unit incorrectly

AlexG1031 added 2 commits February 23, 2024 13:15

Showcase error message if user puts a negative sign in input speed te…

3b2f79a

…xtbox

Units now update properly within analysis panel for ang, angVel and a…

53a7b2f

…ngAcc

Conversation

AlexG1031 commented Feb 12, 2024 • edited by KohmeiK Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented Feb 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for pmksprod ready!

Uh oh!

KohmeiK left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexG1031 Feb 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KohmeiK commented Feb 22, 2024

Uh oh!

AlexG1031 commented Feb 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KohmeiK commented Feb 23, 2024

Uh oh!

KohmeiK commented Feb 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

AlexG1031 commented Feb 12, 2024 •

edited by KohmeiK

Loading

netlify bot commented Feb 12, 2024 •

edited

Loading

AlexG1031 Feb 19, 2024 •

edited

Loading

AlexG1031 commented Feb 23, 2024 •

edited

Loading