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

arrayblit: mark bitmap area as dirty #4430

Merged
merged 1 commit into from
Mar 18, 2021

Conversation

jepler
Copy link
Member

@jepler jepler commented Mar 18, 2021

In #4403 (comment), @dglaude reported that the screen wasn't updating. This is due to forgetting to mark the target of the arrayblit operation as "dirty".

Copy link
Collaborator

@kmatch98 kmatch98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I missed this. I guess this is now the risk we face with the write_pixel function broken out separately.

One note of caution, if a dirty region’s x1 == x2, this signifies that there is no dirty area. Right now there is no checking inside set_dirty_area to enforce x1 != x2. Is there a possibility that x1 could equal x2 here? If so, it will cause unexpected refresh/dirty rectangle behavior for single column changes.

@jepler
Copy link
Member Author

jepler commented Mar 18, 2021

@kmatch98 if you don't mind, I'd like to leave this possible "inefficiency" for now. From what I can tell, the scenario you outline

  • would only affect programs where an arrayblit() call doesn't actually touch any pixels
  • would not create an incorrect display
  • would only cause some pixels to be needlessly redrawn (an inefficiency)

If that's correct, a user who is concerned about the loss of efficiency can simply ensure they don't call arrayblit() to do nothing.

I would like to make some more far-reaching changes to dirty area handling to unify it (and even save a bit of code space!) but it ended up a bit big and would need more testing... which makes me want to do it as a separate pull request

 shared-module/bitmaptools/__init__.c | 61 +++++++++++++++++-----------------------------------------
 shared-module/displayio/Bitmap.c     | 65 ++++++++++++++------------------------------------------------
 shared-module/displayio/Bitmap.h     |  2 +-
 shared-module/displayio/__init__.c   | 48 ++++++++++++++++++++++++++++++++--------------
 shared-module/displayio/area.h       |  3 +++
 5 files changed, 70 insertions(+), 109 deletions(-)

@kmatch98
Copy link
Collaborator

kmatch98 commented Mar 18, 2021

@jepler Maybe I am unclear or am misunderstanding, here goes a scenario where I think it would cause an incorrect dirty rectangle:

  1. Bitmap is created 100 x 100 pixels

  2. The bitmap is manipulated a lot and the dirty_area rectangle covers a big chunk of the area, say (0,0) to (100,100)

  3. Somebody calls arrayblit with x1==x2.

  • No pixels are updated in the bitmap (in this case, the for loops do nothing).
  • The bitmap's dirty_area.x1 and dirty_area.x2 is set to the same value.
  1. Another small update is performed on the bitmap, rectangular region (0,0) to (10,10).
  • This code, sees that dirty_area.x1 == dirty_area.x2, so it updates the final dirty_area to (0,0), (10,10)

To avoid this situation where no pixels are written, when arrayblit gets a zero-size region, it could wholesale skip the for loop (which nothing will happen anyway) and skip set_dirty_area altogether.

I could be too nitpicking this or could be totally wrong so feel free to let me know what I'm missing.

It’s a minor issue overall so if you want to merge I can live with this. And I just looked at your dirty rectangle updates and it looks like those might protect against this kind of weird case.

Copy link
Collaborator

@kmatch98 kmatch98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solves the redraw problem observed.

@tannewt tannewt merged commit c480047 into adafruit:main Mar 18, 2021
@jepler jepler deleted the bitmap-arrayblit-dirty branch November 3, 2021 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants