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

Live range display #8

Merged
merged 38 commits into from
Apr 13, 2023
Merged

Live range display #8

merged 38 commits into from
Apr 13, 2023

Conversation

kerrymilan
Copy link
Contributor

Adds two screens to the controller test ROM:

  • One which plots real-time X/Y values as coordinates on a graph, comparing against either ideal OEM or Hori values, and displays button presses as they occur; and
  • One which provides an oscilloscope-style display of live X/Y values.

Let me know what needs to be changed or cleaned up. I've tested on Everdrive and emu (SimpleN64); testing releases for both screens are published on my fork.

Fixes #5 and, in at least some capacity, #4.

@wermipls
Copy link
Owner

hi! thank you a lot for the pull request :o)

didn't look at the code extensively yet but tested the release from your repo on N64. very nice to see both screens running in full framerate on real console! have a couple nitpicks though, those are mostly in regards to aesthetics/UX.

general:

  • the x/y value display on both screens does not look consistent with how it looks on range result screen (instead of e.g. x: -42 should look like -42 x). at the very least i think it would look nicer with the number aligned to the right

in the live display screen:

  • i expected Z to actually toggle the history display, not toggle and clear it. maybe would be good to add another button to actually clear the buffer?
  • i don't particularly like how the button display works/looks in current form, e.g. start button is not exactly testable. i would remove it for now (and add another screen that lets you test buttons proper, but that's outside of the scope of this PR)
  • with the button display removed, i think it would be fine to center the range display
  • i thought the comparison changing color to magenta was a bug at first. i understand now that it's a way to differentiate between OEM/N64 comparisons, but this makes the meaning of the color inconsistent with the range result screen. i would keep the color green and perhaps make it change the top text similarly to range result screen (so e.g. Live display, ideal Horipad Mini comparison. that's a quite lengthy text though, but should still barely fit when drawn on center. so perhaps it might be wise to indicate the current comparison somewhere else)
  • drawing does not check if the position ends up outside of the screen buffer, this can result in memory corruption with very high stick values (and a near guaranteed crash as a result)
  • using graphics_draw_pixel() instead of graphics_draw_pixel_trans() can probably yield a small performance boost. similarly, fetching the color probably shouldn't be done before every pixel draw lmao. not sure if the compiler actually optimizes it out though

oscilloscope view:

  • with high enough stick values, the plots can overlay each other, but i don't think that's much of a problem for now
  • i think both the plots and the top text should be centered. there's a decent amount of unused space to the right, and a decent leftmost bit of the plots end up outside of action safe area anyway, which makes it not visible on a lot of displays due to overscan. here's a quick idea mockup i hacked up, with red indicating image outside of action safe area, and the orange indicating outside of title safe area, with each area being assumed to be 90% and 80% of the screen size, respectively:
    image
    i think it would be OK to spread the elements wider but since this screen is not very cramped currently (as opposed to range result), i don't see much need to do it.

help screen:

  • control descriptions should be a part of the "basic controls cont." section probably, for consistency.
  • i would do 1024 instead of 1,024, comma as a thousand separator looks kinda awkward from a non-US perspective and the number is not really big enough to need it
  • both d-pad left/right and l/r buttons change the comparison, help only mentions d-pad

on a side note, i think both screens would greatly benefit from increased pollrate, but i think its gonna be a little more complicated to do (and outside of the scope of this PR). they are already plenty useful even in current state to warrant adding them as-is

to be fair i am extremely nitpicky, so feel free to implement only part of my suggestions, or none at all :o) in that case i'll merge and fix up stuff on my own before pushing out a release

@kerrymilan
Copy link
Contributor Author

Wow, thanks so much for the prompt and detailed feedback. I completely agree with all the points you made and will adjust accordingly.

The Z button behavior on the live screen was mostly an artifact left over from testing - I was running into reduced framerates when the history buffer was larger and wanted an option to reset it. It seems to be performing well enough now, so I'll change it so Z toggles the display and something like B clears/resets. I may also tinker with another display mode similar to the bottom half of this pic from the SS64 Twitter.

I can add a third screen to test the buttons. Do you have any thoughts on how to exit that menu? As you pointed out, keeping the behavior consistent with other screens means you can't test the functionality of the start button.

Is the poll rate a function of the efficiency of the event loop, or can you manually increase it from the default? I'm sure there are ways to optimize the history loop, but my C is a tad rusty and I didn't want to get in too far over my head on the first pass.

Would you prefer these three additions be submitted to separate feature branches so you can merge them selectively as they become ready? I kept everything together for now to avoid any merge conflicts in the main menu and help sections.

Again, thanks for the thorough review. I should be able to make these revisions within 24 hours, but I'll let you know if I run into any issues.

@wermipls
Copy link
Owner

from reading libdragon's documentation and code, polling is performed internally by libdragon and seems to be happening on every vertical interrupt, controller_scan() merely synchronizes the cached state. to achieve what i'm thinking about, i think it's necessary to create a timer (running every n milliseconds) that calls async_joybus_read() and then puts the result in an array or so for later processing in the event loop. so e.g. for n = 1 and assuming full framerate, we should get about 17 polls assuming no errors. then we can just draw 17 points on the screen in one frame or plot 17 values etc. i think it's going to be a bit tricky to handle with all the edge cases and stuff so i'd probably want to implement it on my own, as i'm already decently familiar with the joybus protocol and been fighting C memory bugs recently anyway 🤣

the display mode idea sounds nice, probably could be a part of the live display screen since its very similar in concept. will have to use a different approach though (not clearing the image buffer instead of storing history) as my antialiased line drawing routine is not particularly optimized for speed 😅

for button test screen, i would say holding a specific button combination for long enough (e.g. a+b+start for 1 second) would suffice for now. though i think that screen is something i'd like to give more consideration in regards to design. want to have something that looks nice :o) if you still want to do it, i could probably design some mockup to use as a baseline, but yea i think that particular one would be good to be done on a separate branch

@wermipls
Copy link
Owner

btw 1 more thing i forgot about, when you start the oscilloscope screen you can sometimes get an unexpected first plotted value, on first rom boot on real console its pretty random but on next occurences it seems to be leftover from before exiting the screen, perhaps some memory not initialized correctly? also controller_scan() should be called before get_keys_pressed(), but that probably doesn't matter much

@kerrymilan
Copy link
Contributor Author

Cool, I'll leave the async_joybus_read implementation and button display UI design to you. If there are any aspects of those two efforts you want to hand off to me once you figure out the broad strokes, I'm happy to take them on.

If the line display does work out, I think it would make sense as an option to cycle through (e.g. display of history as points, rays, or none). I'm also concerned about performance, though, and will likely not implement it if it's noticeably slower to render than the pixels we're drawing now. Do you think the native graphics_draw_line function would be sufficient here?

I think you're right about plotting the first value before initializing it. The current iteration of that screen doesn't actually show the most recent value since it renders the lines before updating history[0]. I should probably move the calls to draw_aa_line to a separate function and call it on the most recent value.

@wermipls
Copy link
Owner

graphics_draw_line is faster, but not sure by how much. definitely looks much uglier though. i think my routine is fast enough to draw several lines a frame easily (as shown by the comparison in live display anyway), but you'd still really need to overdraw a buffer instead of trying to draw a whole history in one frame. not sure how to create a separate buffer for the overdraws tho, i think you can allocate a sprite_t, then make a surface_t out of its buffer, which you can then pass to the drawing functions (draw_aa_line takes a display_context_t but that seems to just be a typedef for surface_t now) and later draw it as a sprite on the screen. or perhaps just allocate a surface with correct format and manually copy the buffer to the screen surface

that approach means that you essentially have "infinite" history though, which you may or may not like. i'm fine with it

@kerrymilan
Copy link
Contributor Author

Interesting, sounds like a fun side quest. If it's ok with you, I may commit as is for now and reimplement in the coming weeks once I've studied the code some more.

Not sure how I feel about "infinite" history - sounds nice in theory, but I'm not sure how we'd make use of it without a substantially more complicated UI. Was your aim to be able to zoom out and/or trace back beyond the current screen?

@wermipls
Copy link
Owner

my aim is to make it performant enough to be viable :o) since you can just draw 1 line instead of 1000 lines every frame. not considering zooming etc. atm

… B; Update help text to display B usage and note L/R in addition to D-pad
 * Center oscilloscope title text
 * Fix format of x/y coordinates on oscilloscope view
 * Space out oscilloscope elements more logically
 * Adjust zoom to 40% to further limit plot overlap
 * Display current values (history[0]) on plot
 * Center title text on screen
 * Add label indicating current comparison
 * Implement bounds checking on range display
 * Revert `graphics_draw_line` change, using `draw_aa_line` for bounds
   checking
 * Increment counter after render to avoid reading unitialized data
@kerrymilan
Copy link
Contributor Author

Ok, I've got nearly everything implemented with a few exceptions:

  • The live graph is not centered because I'm using the functions in range_test.c to draw the graph and comparison lines, the coordinates for which are hard-coded.
  • I haven't tried tackling the line rendering mode on the live display page; that will have to wait until another day.

I'm open to your input on whether the live range display's title text should be centered, and if we should move the comparison text to a different place. It looks a bit odd being centered when the graph itself isn't, but it needs to be centered in order to fit on one line.

One other behavior on which I'd appreciate your input: if you turn off display of tracking history, should it continue to be tracked? Currently it does not, and resumes tracking when the display is turned back on.

The errant initial values on the oscilloscope display were due to incrementing the counter prior to the first plot render; it was displaying a value that had not yet been initialized. I believe this is fixed but have not yet thoroughly tested it.

I tried using graphics_draw_line instead of draw_aa_line on the oscilloscope page. Ultimately, I left it since your AA function acts as a passthru to graphics_draw_line (since y0 == y1) with the added benefit of handling bounds checking. If you'd prefer I implement bounds checking locally for readability purposes, I'm happy to do so.

Individual fixes are outlined in the commit notes. Let me know if there's anything else you'd like to see changed or cleaned up.

@kerrymilan
Copy link
Contributor Author

Forgot to add - the latest build is attached to the last release here

Copy link
Owner

@wermipls wermipls left a comment

Choose a reason for hiding this comment

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

sorry for the late response, was feeling a bit worn out yesterday. actually properly looking at code this time :o)

other suggestions:

  • might be good to nudge the plots up by 10px to center them vertically on the screen
  • if you still want to try centering the range, i commited adjusted versions of the functions to x-offset branch: b02675f

src/oscilloscope.c Show resolved Hide resolved
src/oscilloscope.c Outdated Show resolved Hide resolved
src/oscilloscope.c Outdated Show resolved Hide resolved
src/range_live.c Outdated Show resolved Hide resolved
src/range_live.c Outdated Show resolved Hide resolved
src/range_live.c Show resolved Hide resolved
src/range_live.c Outdated Show resolved Hide resolved
src/range_live.c Outdated Show resolved Hide resolved
src/range_live.c Outdated Show resolved Hide resolved
 * Shorten label text
 * Call controller_scan() before get_keys_pressed()
 * Center elements horizontally
 * Whitespace cleanup
 * Implement zoomout to 75% similar to range display page
 * Adjust controls for consistency:
    * A: toggle history display
    * Z: toggle zoom
 * Update help text
@kerrymilan
Copy link
Contributor Author

Ok, I think I've got these changes implemented. I also ported the range display page's zoom functionality to the live display since centering the graph introduced an overlap with the X/Y labels. Thanks for your patience; let me know if there's anything else that needs to be changed. Test build here.

Copy link
Owner

@wermipls wermipls left a comment

Choose a reason for hiding this comment

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

looks good! likewise, thank you for your patience and your work :o)

@wermipls wermipls merged commit 6126231 into wermipls:main Apr 13, 2023
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.

Add an oscilloscope view
2 participants