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

Drawing within a clipping region #769

Closed
ImpulseAdventure opened this issue Oct 2, 2020 · 13 comments
Closed

Drawing within a clipping region #769

ImpulseAdventure opened this issue Oct 2, 2020 · 13 comments

Comments

@ImpulseAdventure
Copy link
Contributor

Hi @Bodmer -- fantastic work as always on the library!

I am wondering if there may be a way to render a BMP (eg. pushImage()), but only transfer a rectangular region of the overall image? In other words, I would like to perform pushImage, but constrained within a target clipping rectangle.

We already benefit from your optimized rendering in the GUIslice GUI, but such a feature would enable fast rendering during partial screen redraws (when buttons and visual controls are updating), as well as benefit the general TFT_eSPI user-base who may desire even faster updates.

I had originally thought that setAddrWindow / setWindow could be used, but of course these are already leveraged during row rasterization. Providing an offset to the top-left corner of the clipping region into the data param won't properly constrain dw, etc. Is there an alternate method for users to define a clipping region without leveraging offscreen/sprites?

Note that I have locally created a modification to TFT_eSPI that adds a setClipRect() API which can constrain the ensuing calls to pushImage() (and optionally other APIs where you are currently clipping on the screen boundaries). Seems to work quite well, in case you are interested in a PR.

Thanks!

@Bodmer
Copy link
Owner

Bodmer commented Oct 2, 2020

If the image is stored in a Sprite then there is a function to render a window of the Sprite to the TFT. This works with 16,8,4 and 1 bit per pixel images.

See #712

@ImpulseAdventure
Copy link
Contributor Author

Hi @Bodmer -- thanks for pointing out a possible approach!
Yes, I did note the option to use sprites, but my hope was that the clipping would not depend on the use of an offscreen bitmap.

IMHO, it looks like it should be relatively straightforward to add this capability with only minimal changes.
If you are interested in a pull request for this feature, I have submitted one for consideration in #770 .

@Bodmer
Copy link
Owner

Bodmer commented Oct 4, 2020

OK, I thought you were looking at just image cropping. I have a code branch that implements TFT display viewports with full cropping in all functions but this has not been fully tested. This was created so the TFT datum can optionally be moved to the viewport corner for menus, virtual screens etc, but if it the datum is not moved it would give the functionality you ask for.

@Bodmer
Copy link
Owner

Bodmer commented Oct 5, 2020

I have created a "Viewport" branch that includes the viewport capability. I have included an option to allow the coordinate datum to remain at the top left corner of the screen. By default it relocates to the top left corner of the viewport.

Let me know if this provides the solution you are looking for. I think all graphics functions now crop at a window but let me know if you find any issues.

@ImpulseAdventure
Copy link
Contributor Author

Thank you very much Bodmer!

Yes, I see now that my original post had specifically mentioned bitmaps (as they offer the best acceleration opportunity) though I regarded a more generalized clipping behavior as the ideal solution.

Your viewport implementation appears to be quite similar to my PR, so I am pretty sure it will address the original intent very well. Let me do some additional testing on your branch and send you a followup.

Thanks again for the quick update on this, Bodmer!

@ImpulseAdventure
Copy link
Contributor Author

ImpulseAdventure commented Oct 6, 2020

One difference I noted from a quick read of the viewport branch code (note that I haven't had an opportunity to test yet):

  • setViewport() accepts x,y,w,h
  • _vpW, _vpH are treated as "width" and "height" respectively
  • However, in the clipping code that follows (setClipRect branch on the left, viewport branch on right), it seems like _vpW is treated as an "X1" (right edge coordinate) rather than a "width". For example:
    image

The above should work if _vpX were zero, but does it still work if _vpX were non-zero? My guess is that the clipping might not occur at the right viewport boundary... though perhaps I misunderstood the definition of the "w" and "h" parameters to setViewport() when x or y are non-zero.

For example, if _vpX=160 and _vpW=100, I would expect the active viewport range would be X=160..259. However, I wonder if the code above might instead mask any X>=100?

Feel free to disregard the above until I have had a chance to actually test it out, but my brief look at the code makes me think that we might need to adjust the way the clipping is treated on the right/bottom extents.

@Bodmer
Copy link
Owner

Bodmer commented Oct 6, 2020

I understand the confusion the _vpW and vpH are the right hand clip coordinate + 1. The reason for the naming is a reminder of the original clip code which used the width and height of the TFT, e.g. say 240 and 320. The coordinate range is the 0-239 and 0 319. So width and heigh of the TFT were 1 more than the coordinate range. This is convenient as otherwise you need to use +1 sometimes in the clip code and it can get confusing where this is needed.
Comparing the original clip code:

  if (x < 0) { dw += x; dx = -x; x = 0; }
  if (y < 0) { dh += y; dy = -y; y = 0; }

  if ((x + dw) > _width ) dw = _width  - x;
  if ((y + dh) > _height) dh = _height - y;

To the new:

  if (x < _vpX) { dx = _vpX - x; dw -= dx; x = _vpX; }
  if (y < _vpY) { dy = _vpY - y; dh -= dy; y = _vpY; }

  if ((x + dw) > _vpW ) dw = _vpW - x;
  if ((y + dh) > _vpH ) dh = _vpH - y;

You can see that vpW and vpH simply replace the original width and height, so the change is very simple. I agree the name is a bit of a misnomer without knowing the history, but it is not a variable directly accessible to a user.

I have not performed exhaustive tests but as the original code worked it should be OK.

The only restriction with this simple change/clipping approach is that a negative x or y for the viewport does not produce the desired result, so the viewport function resets a negative x or y to zero. However I don't think this will affect your application of the function.

@ImpulseAdventure
Copy link
Contributor Author

ImpulseAdventure commented Oct 8, 2020

Hi @Bodmer -- thank you for the explanation... the _vpW / _vpH naming was indeed a little confusing, but given that these are private, I agree it should be less of a concern.

I was a bit slower to provide my testing feedback as I had discovered that my Adafruit Feather ESP32 appeared to stop working with the recent TFT_eSPI releases. I tracked this down to a side-effect of commit cd10a92 . I may post a separate issue to capture a couple details in case they are helpful for others.

As for the viewport: good news! I did some tests with your setViewport() code functionality integrated into GUIslice and it is working very nicely so far. Thank you for integrating this so quickly! We can now support very fast localized control updates with this change. So far the functionality looks correct in my testing (a couple directed testcases in addition to integration within the GUI's redraw operations).

In order to facilitate backward compatibility, would it be possible to add a define so that user code can programmatically determine whether the new API is available? For example (in TFT_eSPI.h v2.3.0 onwards): #define SUPPORT_VIEWPORT

Your viewport enhancement will add a lot of value to those TFT_eSPI users with more advanced needs.

thanks!

@Bodmer
Copy link
Owner

Bodmer commented Oct 10, 2020

I am updating the branch so that any viewport position will work (negative x,y cause probelms with the current branch. This will not change the API. The viewport concept has been in the development pipeline for a while so your query has brought it to the top of the list! I just neet to test some corner cases.

@Bodmer
Copy link
Owner

Bodmer commented Oct 11, 2020

Found a few issues so the viewport branch has been updated.

I have performed many tests and this new version looks to be bug free and fully working so will be merged soon.

@ImpulseAdventure
Copy link
Contributor Author

Great, looking forward to it!

Do you think it might be possible to add a simple #define line to enable compile-time API availability detection to this release? (I don’t think one can easily compare for a minimum TFT_ESPI_VERSION at compile time). Thanks!

@Bodmer
Copy link
Owner

Bodmer commented Oct 13, 2020

I expect there will be more features added so I think this will be OK:

// Bit level feature flags
// Bit 0 set: viewport capability
#define TFT_ESPI_FEATURES 1

@Bodmer
Copy link
Owner

Bodmer commented Oct 13, 2020

Changes have been made and pushed to the master branch.

Please raise a new issue if you discover problems.

@Bodmer Bodmer closed this as completed Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants