Remove use of virtual functions

[joshgoebel]

Pasting my comment from the other issue. It was a bit off topic there.

Please read: http://www.learncpp.com/cpp-tutorial/122-virtual-functions/

We could also just remove the "virtual". This isn't really the intended use for virtual - since there is no polymorphism happening here. Someone is going to subclass GFX and then all the calls are going to be against that class. You aren't going to have multiple AdaFruitGFX subclasses being passed around where the function dispatch needs to happen at runtime. The function dispatch can all be resolved statically at compile time, the unused functions can be discarded, and the virtual dispatch table is also wasted space that will be saved.

---

I just did a test now with the Arduboy lib (in my branch). I have core class that the main class builds on... very low-level hardware stuff is in core (not even drawPixel), graphics stuff is in the subclass (including drawPixel). The Ardubreakout sketch compiled (before any changes) is 17,498.

I modified core to add this single line:

# subclass must implement this virtual function
virtual void drawPixel(int x, int y, uint8_t color) = 0;

That's the only change. The compile size quickly jumps to 17,592. 94 bytes larger just for dynamic dispatch of a single function. That's not even counting the increased overhead of virtual function dispatch.

Thoughts?

[joshgoebel]

Also good reading on the cost of virtual dispatch:

https://hackaday.io/post/18729

[tdicola]

Yeah this is tricky since we're using subclassing/inheritance here to prevent a lot of code duplication. All the logic of drawing shapes etc. live in the GFX class and are coded to use the pure virtual drawPixel function like it's a little interface. That way we don't need to copy all the same line, box, circle, etc. drawing code to every display library, instead the display libraries inherit from GFX and tell it how to draw pixels on the display. The use of inheritance here is really more for easier maintenance and code reuse.

Another way to do this could be to use composition instead of inheritance. I.e. have the GFX class take a pointer to a class that has a drawPixel function and use that to call drawPixel in all the drawing routines. I don't think we'll really see any difference though since you're effectively implementing your own vtable type of lookup.

Something that might work would be to use compile time polymorphism with some template magic (like https://en.wikipedia.org/wiki/Curiously_recurring_template_pattern), but that's going to add a lot of complexity and scary error messages/template spew when things go wrong. It also could be problematic on old versions of Arduino that use g++ ~4.3 and only support templates circa C++ '98.

Ultimately I think the flexibility & code reuse is worth it for the ~100 byte hit to do some dynamic dispatch. I think you'll get much better space savings with https://github.com/adafruit/Adafruit-GFX-Library/issues/50 and turning off the print class inheritance. If that's still not enough then you probably need to spin off GFX drawing functions into an Arduboy specific drawing library. That would give you the flexibility to rip out anything that isn't absolutely necessary and eke out the max space savings.

[joshgoebel]

Yeah this is tricky since we're using subclassing/inheritance here to prevent a lot of code duplication.

It could be I'm misunderstanding C++... but what happens if just you:

1. Remove virtual
2. Make a dummy stub for drawPixel (if the compiler needs this)

If I subclass and only use my subclass then everything should work. What am I missing? All the issue's I've read where you really need virtual involve stuff like Shape s = (Triangle) y; and whether Shape's methods or Triangles are called.

Since GFX is only intended to be subclassed and then the subclass used I'm not understanding why the virtual functions are needed at all. If they were not virtual the subclasses functions should be linked and called if you're working with an instance of that subclass.

What am I missing?

[joshgoebel]

What's the best example code to use to compile something against GFX? Use SSD1306 examples? I can muck with it and see if my assumptions his some wall.

[tdicola]

Oh you can actually try it to see the issue, but if you remove virtual then you'll get an error that the GFX class can't find the drawPixel function. If you add a stub drawPixel function to GFX then the GFX class will compile but you'll get an error compiling the display class that it's redefining drawPixel. If a class wants to redefine drawPixel it has to be marked as virtual so the compiler knows that it should look up the drawPixel function in the child class.

Yeah any display class would work to test thinga out--SSD1306 works, or the Nokia LCD class is pretty simple too: https://github.com/adafruit/Adafruit-PCD8544-Nokia-5110-LCD-library

[joshgoebel]

l. If a class wants to redefine drawPixel it has to be marked as virtual so the compiler knows that it should look up the drawPixel function in the child class.

That's what I haven't run into.

[joshgoebel]

Ok, yeah that's annoying. So it will compile just fine... and if you use object.drawPixel you get the correct one... but any methods on the base class are going to call the base classes drawPixel... so object.drawCircle will use the empty/wrong drawPixel. Grrr.

N/m, I don't understand yet. It seems to be linking no code at all for drawPixel now when it's referenced without an explicit caller.

[joshgoebel]

If you add a stub drawPixel function to GFX then the GFX class will compile but you'll get an error compiling the display class that it's redefining drawPixel.

I can't reproduce this. I get no error. It just compiles and silently seems to drop a lot of drawPixel calls (they are missing from the assembly).

@tdicola If the stub drawPixel is declared static it seems to keep the compiler happy and the subclasses non-static member function drawPixel seems to be linked everywhere that drawPixel is called. I don't have a device handy, but the disassembly looks good.

Can I make a PR just to show you what I'm doing and get further feedback?

[joshgoebel]

I don't think we'll really see any difference though since you're effectively implementing your own vtable type of lookup.

Right, except the linker would always be able to figure out what is and isn't used and remove them from the final output.

[joshgoebel]

Actually now the static doesn't seem to make any difference at all.

[joshgoebel]

https://github.com/yyyc514/Adafruit-GFX-Library/commit/dfa00503a7cd2eca77628790400bd3d6a512f10d

IE, this seems to work just fine for me.

SSD1306 also replaces drawFast*Line methods and those also seem to link just fine - overwriting those from the base class without any errors or warnings that I see.

Am I missing something?

[tdicola]

In your example are you only calling the drawLine, drawFastVLine, etc. functions that the subclass implements? If so it might work since the subclass has those functions. However try calling drawCircle to see what happens when the base GFX class is calling its non-virtual drawPixel stub. You'll get the stub being called and not the drawPixel from the display subclass.

A static drawPixel is tricky since you might use two different displays in the same sketch (like a OLED + LCD, etc). In that case the static drawPixel that each defines would collide.

[joshgoebel]

In that case the static drawPixel that each defines would collide.

Forget what I said about static. It works just fine without static.

In your example are you only calling the drawLine, drawFastVLine, etc. functions that the subclass implements? If so it might work since the subclass has those functions. However try calling drawCircle to see what happens when the base GFX class is calling its non-virtual drawPixel stub. You'll get the stub being called and not the drawPixel from the display subclass.

No, I'm using the test program provided with SSD1306. I uses almost all the drawing methods. The drawPixel that gets linked everywhere is the one from SSD1306, not the stub from GFX.

[joshgoebel]

Simple case:

http://ideone.com/lx3cl7

Is something there wrong?

[joshgoebel]

Ok, I updating my example to show the problem, but for some reason with SSD and GFX the same thing actually seems to be working.

[PaintYourDragon]

The C++11 'using' keyword might work as a substitute.

In Adafruit_GFX.h, declare drawPixel() as non-virtual. Make a function in the .cpp that accepts the usual arguments but is empty. In subclass, implement 'real' drawPixel(), and precede it in the header, inside the class declaration, with 'using Adafruit_GFX::drawPixel;' This checks the base class for a function first, but then overrides with the subclass version...sort of what the 'virtual' technique does, without the virtual. Saves a little code space too (about 120 bytes or so on AVR). More recent GFX versions have added a whole bunch of virtual functions, with corresponding bloat, so there's potential for big savings.

Just did this for a feature in the ILI9341 library (but for a specific drawRGBBitmap() variant instead of drawPixel()) and seems OK, does the Expected Thing.

Downsides: every GFX subclass would need to be updated, and unsure of compatibility with compilers for third-party boards (if they use an archaic pre-C++11 compiler).

[pljakobs]

also, since I'm working on using multiple displays (which works well currently) would that pose any issues?

[joshgoebel]

Forget what I said about static. It works just fine without static.

Other than adding static I don't think virtual or non-virtual would have any effect on multiple instances.