pimoroni / picosystem

PicoSystem libraries and examples.
MIT License
145 stars 33 forks source link

Allow `const` source buffers when drawing #95

Open joewreschnig opened 1 year ago

joewreschnig commented 1 year ago

This is one step towards allowing constexpr buffer_ts, saving some RAM and probably compiling a little more optimally.

ahnlak commented 1 year ago

Can you explain to a "C++ is just C with classes" level why allowing constexpr buffer_ts is desirable (and for bonus marks, how it saves RAM)?

I'm sure it's super obvious to a serious C++ brain, but I'm not incredibly familiar with the subtleties of constexpr.

(other than that; declaring stuff that obviously should be const as const is always a good idea!)

joewreschnig commented 1 year ago

Objects marked constexpr (and results of constant expressions generally, but the more you use constexpr the more constant expressions you can easily write) can go into the .rodata executable section, which ends up in flash and not RAM.

This change isn't sufficient to do that for buffers yet (and unfortunately I'm not sure it will ultimately be possible with the current API), but it's a necessary first step.

ahnlak commented 1 year ago

But if you're creating a buffer_t with constexpr content, that data chunk will still be in .rodata - and obviously there are lots of buffer_t instances which are very much not constant.

If it's possible to get there without API-breaking changes I'm all for it - and these changes make sense regardless.

joewreschnig commented 1 year ago

Without these changes, you can keep the pixel data (admittedly, by far the largest part of a buffer) in rodata but not the buffer itself. If the buffer is constant, then the 16 bytes of the buffer itself are also in rodata. There may be a speed hit from loading them from flash, but you also win some back anywhere the compiler now avoids a load completely.

You can do this somewhat easily in C++20 already because it introduces constexpr destructors, and since no such buffer will have .alloc = true it should be safe. (Even some other cases maybe - I'm not up-to-date with the workings of C++20 constexpr new/delete.) Although you are still stuck stripping a const from the pixel data. A wrapper even makes the syntax quite nice by automatically inferring the buffer size from the data size, e.g. after

modified   picosystem/libraries/picosystem.hpp
@@ -43,7 +43,7 @@ namespace picosystem {
       return data + (x + y * w);
     }

-    ~buffer_t() {
+    constexpr ~buffer_t() {
       if (alloc) delete data;
     }
   };

then

template <int32_t W, int32_t H>
constexpr picosystem::buffer_t buffer(const picosystem::color_t (&spr)[H][W]) {
    return {
        .w = W, .h = H,
        .data = const_cast<picosystem::color_t*>(&spr[0][0]),
    };
}

constexpr picosystem::color_t sprites[128][128] = { ... };
constexpr picosystem::color_t background[120][120] = { ... };
constexpr auto
    sprites_buf = buffer(sprites),
    background_buf = buffer(background);

I'm not sure how to do it before C++20 without shifting the memory management to a separate, non-constexpr'able class.