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

Feature/#151 time sweep indicator #215

Merged
merged 7 commits into from
Feb 5, 2023

Conversation

jenswaechtler
Copy link
Contributor

@jenswaechtler jenswaechtler commented Feb 2, 2023

Changed the indicator and fixed the formatting of other files via npm run format
Looks like this after the change.
grafik

nicolaskolbenschlag and others added 5 commits December 27, 2022 09:59
Signed-off-by: nicolaskolbenschlag <[email protected]>
Signed-off-by: nicolaskolbenschlag <[email protected]>
…p-indicator

# Conflicts:
#	Apps/frontend/src/components/TimeSweepSlider.svelte
#	Apps/frontend/src/components/WaveControls.svelte
#	Apps/frontend/src/components/Waves.svelte
#	Apps/frontend/src/helper.js
#	Apps/frontend/vite.config.js
Signed-off-by: Jens Wächtler <[email protected]>
Signed-off-by: Jens Wächtler <[email protected]>
@cypress
Copy link

cypress bot commented Feb 2, 2023

Passing run #580 ↗︎

0 42 0 0 Flakiness 0
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Details:

Feature/#151 time sweep indicator
Project: sosci-frontend Commit: b5f7300e10
Status: Passed Duration: 04:18 💡
Started: Feb 4, 2023 1:09 PM Ended: Feb 4, 2023 1:13 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

export let controlPanelBottomHeight = 0;

const computeDisplaySpeed = (value) => {
let delta = computeDisplayDeltaFromTimeSweep(value);
return ((1000 * TIME_PER_DIV) / delta).toFixed(2) + " ms/div";
return (1 / delta).toFixed(2) + " s/div";
Copy link
Contributor

@nicolaskolbenschlag nicolaskolbenschlag Feb 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Danke, dass du dir das mal angesehen hast. Ich bin mir jedoch nicht sicher, ob das richtig sein kann. Die Bestimmung des Display-Speed hängt jetzt nicht von der Update-Frequenz der Signale ab. Aber das müsste es ja. delta ist nur ein Faktor zwischen 0 und 2.

Beispiel:
Wenn jetzt unser Generator doppelt so schnell senden würde (z.B. EXPECTED_UPDATES_PER_SECOND =20_000), würde sich der Display-Speed nicht verändern. Aber nach meinem Verständnis sollte sich in dem Beispiel die Display-Speed dann halbieren.

Ich vermute, dass der Bug der noch drin ist, eher daher kommt dass in der Konstante CANVAS_WIDTH nicht die Anzahl der Pixel steht, sondern irgendeine andere Maßeinheit. Wir müssten also die Anzahl der Pixel/der Updates in der Horizontalen irgendwie anders herausfinden.

Mal sehen was die anderen sagen. Von mir aus kann man es wegen der wenigen verbleibenden Zeit auch so lassen, damit die Zeiteinheit am Demo-Day immerhin richtig aussieht, aber rein fachlich passt das so noch nicht (glaube ich).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ich hab es mal adressiert, und für die zukünftigen Bearbeiter einen Kommentar hinterlassen.

Copy link
Collaborator

@motschel123 motschel123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mit Augenmaß und Stoppuhr gemessen, passen die werte. IMO reicht das für die Demo wenn wir eine fixe Datenrate erwarten.

Copy link
Collaborator

@PhlppKrmr PhlppKrmr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was quite hard to see what exactly changed, but thanks for linting ;)
Same opinion as Marcel, looks good (enough)!

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

Successfully merging this pull request may close these issues.

4 participants