espruino / Espruino

The Espruino JavaScript interpreter - Official Repo
http://www.espruino.com/
Other
2.73k stars 741 forks source link

overlays are cut off from the top instead of landing at desired y position #2520

Closed ssievert42 closed 1 week ago

ssievert42 commented 1 week ago

Overlays are being cut off from the top, by about as many pixels as their y position should be, instead of landing at the desired y position.

To reproduce on a Bangle.js 2 (code copied from the docs about overlays):

g.setBgColor(0, 1, 0).clear().reset();

var ovr = Graphics.createArrayBuffer(100,100,2,{msb:true});
ovr.transparent = 0;
ovr.palette = new Uint16Array([0,0,g.toColor("#F00"),g.toColor("#FFF")]);
ovr.setColor(1).fillRect({x:0,y:0,w:99,h:99,r:8});
ovr.setColor(3).fillRect({x:2,y:2,w:95,h:95,r:7});
ovr.setColor(2).setFont("Vector:30").setFontAlign(0,0).drawString("Hi",50,50);
Bangle.setLCDOverlay(ovr,38,38, {id: "myOverlay", remove: () => print("Removed")});

Expected outcome: good

Actual outcome: bad

A git bisect, using the emulator, leads to this being the first bad commit: fdbf0bfd2590bf241eabe3877b283a78391035ef

Moving the following line inside the for loop above it seems to fix things https://github.com/espruino/Espruino/blob/31c5d60eb743587d9b505b5e7403eb4d8567eda1/libs/graphics/lcd_memlcd.c#L347 but that would revert part of 5a99fbdafedf5b8a551ac53f2370c76f63be9523 I've got absolutely no idea when partial updates happen, but I guess moving that iterator call inside the for loop would probably break something else ¯\_(ツ)_/¯

ssievert42 commented 1 week ago

I did some more digging: Since fdbf0bfd2590bf241eabe3877b283a78391035ef changed the positions of a GfxDrawImageLayer to be the value in pixels, shifted to the left by 8 bits, we need to shift the desired overlay y position by 8 bits before assigning it to the GfxDrawImageLayer's y1. I did just that in #2521, and it looks like that did the trick :)

ssievert42 commented 1 week ago

I completely forgot that overlay positions can be negative :sweat_smile: Multiplying the pixel position by 256 (which I did in #2522) is probably a lot more readable anyways.