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

[graphic] Rewrite graphics - Round 1 #665

Closed
wants to merge 1 commit into from

Conversation

TomSaw
Copy link
Contributor

@TomSaw TomSaw commented Jul 29, 2021

Synopsis of added goods

  • Generic colors
    • Feature complete
      • All operations with saturation arithmetics
      • Conversion between any instances of Hsv <-> Rgb
    • Bit granularity for modm::Gray. Same for modm::Rgb and modm::Hsv cause they're made form 3x modm::Gray
      • Symetric scaling. Example convertions 2-bit Gray ---> 4-bit Gray.
        • 0b00 --> 0b0000
        • 0b01 --> 0b0101 // No stupid lshift, the gap is filled up
        • 0b10 --> 0b1010
        • 0b11 --> 0b1111 // Saturated stays saturated, white stays white
    • C++20 concepts for colortypes and colorgroups (currently ColorPlane and ColorPalletizing). These concepts are very handy to modelate overload resolutions on specialision of buffers and their methods. They're also the key for good readability.
  • Generic graphic buffers
    • Arbitrary Color and Resolution
    • Full support for Buffer interoperation: Copy/Convert buffers from sources (Flash, Ram, SD-Card not yet implemented) to destinations (Ram, Display).
    • Pixel palletizing for 1-, 2- and 4-bit grayscale (1-bit == Monochrome)
    • Indexed colors for compressed, canned Images but also for resource-friendly drawing and texting.
  • Buffer manipulations
    • Optimized drawing of primitives (Point, Line, Circle, Ellipse)
    • foregroundColor, backgroundColor -> replaced by a colormap*
    • DMA whenever possible: each of clear() and Buffer copies without color-conversion / -mapping
    • Generic fill patterns (gradients, checkerboard, stripes). See examples
  • Support external hardware
    • All features also work efficiently on external hardware using resumable functions: Currently implemented for ili9341
    • ili9341::draw(Point / HLine / VLine / Rectangle, Section) is hyper-fast cause no pixel-pushing is involved. Just some register setup and DMA, filling the area. See Teach DMA to transfer n bytes n times #666 . This is great for Prototyping and Tools: You can f.e. draw/update colored Bars with next to zero resource-footprint.

Roadmap

TODO

  • Pass Buffer<...> in RAM or Flash using accessors
  • Support arbitrary non-grayscale Buffers
  • Support arbitrary grayscale Color and Buffers
    • color::GrayT<...> implementation to cover f.e. 4 and 16 level grayscale displays.
    • graphic::Buffer<GrayT<...>, ...> implementation. Targets pixel stacking algorithms.
  • Specialise Buffer<Monochrome, .. > for speed
  • lbuild options
    • Disable modm_asserts in GrayD constructors
    • Visualize refresh for Display-drivers ( Similar to Android Developer option )
  • Add/Update tests
    • Buffer conversion
  • New examples
    • Simplest to advanced rendering
    • Custom graphic::LocalPainter with simple UI widgets like Button, Checkbox and Slider
    • Generic Tetris Game as rich overall demo

Here's a list for follow up rounds.

Rewrite Graphics - Round 2

  • Support STM32 DMA2D
    • : Compose a new modm::Buffer from multiple modm::Buffer like layers in graphics manipulation software.
    • Porter & Duff composing. Conversion-constructors for each in modm::ui::color::* but also depends on what DMA2D can do. See uPainter
  • Canned images on SD-Card
  • Drawing
    • Dithering
    • Anti-Aliasing
    • Line-thickness and Line-style (Dotted, Dashed)
    • More shapes

@TomSaw TomSaw mentioned this pull request Jul 30, 2021
@TomSaw TomSaw changed the title Rewrite Graphics API Rewrite Graphics API (Whole code as reference) Jul 30, 2021
@TomSaw TomSaw force-pushed the rewrite-graphics-API branch 4 times, most recently from b81fd96 to f9c5077 Compare July 30, 2021 07:21
@TomSaw TomSaw changed the title Rewrite Graphics API (Whole code as reference) Rewrite Graphics API Jul 30, 2021
@TomSaw TomSaw changed the title Rewrite Graphics API [graphic] Rewrite Graphics API Jul 31, 2021
@TomSaw TomSaw force-pushed the rewrite-graphics-API branch 2 times, most recently from 093aa98 to dd857ab Compare July 31, 2021 10:13
@TomSaw TomSaw marked this pull request as draft July 31, 2021 16:44
@TomSaw TomSaw force-pushed the rewrite-graphics-API branch 4 times, most recently from f2326c6 to 60c8450 Compare August 1, 2021 17:12
@salkinium
Copy link
Member

It's the exams phase so I don't have a detailed review, but here are some rought points:

  • Overall: Yes, very nice!
  • Don't put an endianess template parameter in the color format, especially if you don't even store the underlying data in said endian format. Instead use modm::toBigEndian/modm::fromBigEndian inside the buffer functions.
  • I'm unsure about the Resumable interface. I see the issue with the SPI transfers for the off-device frame buffer of the Ili9341, however there's a lot of duplication with RF_CALL stuff, that would fall away when we switch to fibers.
  • Not sure why the Buffer needs to know the type of the Painter? I think the buffer should only abstract the resolution/data format/pixel access and the Painter should access that via a virtual interface (which may include optimizations like your fast methods).
  • The .font files and the SConscript.generate can be deleted as they are duplicated in modm/tools/font_creator/fonts, however, the ubuntu_36.font should be copied there. Perhaps in future we could replace the modm/tools/font_creator with something better.

@TomSaw TomSaw force-pushed the rewrite-graphics-API branch 2 times, most recently from 8d11b57 to febf47d Compare August 2, 2021 19:40
@TomSaw
Copy link
Contributor Author

TomSaw commented Aug 2, 2021

Thanks @salkinium for the quick review. I'm happy with such short mentoring, Nice to hear the code isn't bullshit and thanks for the feedback.

... especially if you don't even store the underlying data in said endian format

It pays out to handle the Color in delivery format from minute one. The Endian feature in Rgb565 is not yet completely implemented but I do store it in said format and works as expected. Another optimal solution arised - no more need to adjust endianes. See my latest comments in #666.

Buffer needs to know the type of the Painter?

Painter manipulates Buffer / Display only via one of drawFast(...) because the memory-mappings of monochrome Buffer / colored Buffer / Display work so differently. I don't wanted a composition-coupling via pointer between Painter and Buffer. For max. speed, Painter needs to be a Base to Buffer / Display with virtual drawFast(...) methods.
EDIT: Took me a while to see 🧐 I had to swap the inheritance 🤓:

Before: Buffer/Display-> Painter, Now: Painter->Buffer / Display

Advantages

  • Inline instead of virtual calls of drawFast(..) makes drawing ~1-20% faster
  • Template declarations of Buffer<...> and it's methods simplified
  • RemotePainter->Ili9341 now works with Resumable Functions! Hurray

The declarations of LocalPainter / RemotePainter are ugly for now. Working on this...

@TomSaw TomSaw force-pushed the rewrite-graphics-API branch 6 times, most recently from 8f6dcfa to 2ed0fbb Compare September 14, 2021 14:37
@TomSaw TomSaw changed the title [graphic] Rewrite Graphics API [graphic] Rewrite Graphics Sep 15, 2021
@TomSaw TomSaw changed the title [graphic] Rewrite Graphics [graphic] Rewrite graphics Sep 15, 2021
@salkinium
Copy link
Member

I'm generally positively impressed by these changes and looking forward to the finished PR, particularly since there are many very cool improvements in here! 😍

However, I've completely lost the overview of this PR and GitHub is struggling to show me the whole diff, due to +6,742 −14,976 changes, so I'm struggling to review it… There are also too many commits for me to click through, so I cannot review commit by commit either.

I think we need to split this up a little, are there changes you can pull out into separate PRs that are more independent? There's no time pressure, just want to let you know that I'm willing to review just not really able to.

@TomSaw TomSaw changed the title [graphic] Rewrite graphics [graphic] Rewrite graphics - Round 1 Sep 22, 2021
@TomSaw
Copy link
Contributor Author

TomSaw commented Sep 22, 2021

Quote @salkinium, 30 .Juli 2021 in #660 😜

Just make one big PR and partition your changes into separate commits

Also didn't expect such a code explosion - It's much fun, a great learning subject and ongoing story: Let's keep #665 as reference and merge proven pieces step bz step! Here's Nr.1 #690

are there changes you can pull out into separate PRs that are more independent

Yes. I have renamed the ordered list in my opening message: Planned commits -> Planned PRs.

@TomSaw TomSaw force-pushed the rewrite-graphics-API branch 2 times, most recently from b8d05e6 to e9f7689 Compare October 4, 2021 10:21
@TomSaw TomSaw mentioned this pull request Oct 5, 2021
10 tasks
@TomSaw TomSaw force-pushed the rewrite-graphics-API branch 2 times, most recently from f2f625d to fd926fa Compare October 31, 2021 22:27
@TomSaw
Copy link
Contributor Author

TomSaw commented Nov 15, 2021

I've created PRs for environmental changes @salkinium so we get the table as clean as possible to concentrate on the real chunks. See section Roadmap in this PRs opening message.

@TomSaw
Copy link
Contributor Author

TomSaw commented Nov 19, 2021

I think we need to split this up a little, are there changes you can pull out into separate PRs that are more independent? There's no time pressure, just want to let you know that I'm willing to review just not really able to.

Closed because PR is way too big. First fragments are in review and you find the rest of the code in my origin feature/rewrite-graphic

@TomSaw TomSaw closed this Nov 19, 2021
@TomSaw TomSaw deleted the rewrite-graphics-API branch November 19, 2021 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants