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

Add sync id and tile count to raster tile messages #1288

Merged
merged 9 commits into from
Aug 22, 2023
Merged

Conversation

pford
Copy link
Collaborator

@pford pford commented Aug 2, 2023

Description

  • What is implemented or fixed? Mention the linked issue(s), if any.
    Add sync_id to tile messages #1282
  • How does this PR solve the issue? Give a brief summary.
    With multithreading, there could be overlapping RasterTileSync/RasterTileData messages. Adding an id and a count to these messages helps the frontend sort them out.
  • Are there any companion PRs (frontend, protobuf)?
    frontend #2219, protobuf #87
  • Is there anything else that testers should know (e.g. exactly how to reproduce the issue)?
    See frontend issue #1988
    Checklist
  • changelog updated / no changelog update needed
  • e2e test passing / added corresponding fix
  • protobuf updated to the latest dev commit / no protobuf update needed
  • added reviewers and assignee
  • added ZenHub estimate, milestone, and release

@pford pford added requiring frontend this backend issue requires work in both frontend and backend requiring protobuf labels Aug 2, 2023
@pford pford added this to the v4.0-stable milestone Aug 2, 2023
@kswang1029 kswang1029 self-requested a review August 2, 2023 02:39
Copy link
Contributor

@kswang1029 kswang1029 left a comment

Choose a reason for hiding this comment

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

the solution with sync_id works nicely as far as I can tell. No missing tiles after animation stopping, or during animation playback. Vector overlay is rendered properly during/after animating as well. 👍 Thanks @crocka and @pford.

@acdo2002
Copy link
Contributor

acdo2002 commented Aug 4, 2023

Because adding the syncId for RASTER_TILE_DATA and RASTER_TILE_SYNC, the ICD tests need to be updated, please merge it after finishing the ICD tests (otherwise the daily monitor tests will failed). I will let everyone know when the tests are finish the updates.

@acdo2002
Copy link
Contributor

I have updated the tests code PR that can be passed on MacOS11 with this backend PR (and corresponding protobuf).
Thanks for waiting!

@github-actions
Copy link

Code Coverage

Package Line Rate Health
src.Cache 66%
src.DataStream 52%
src.FileList 67%
src.Frame 50%
src.HttpServer 43%
src.ImageData 28%
src.ImageFitter 89%
src.ImageGenerators 44%
src.ImageStats 76%
src.Logger 44%
src.Main 54%
src.Region 18%
src.Session 29%
src.Table 52%
src.ThreadingManager 87%
src.Timer 85%
src.Util 48%
Summary 38% (7012 / 18558)

@confluence confluence merged commit aa5df48 into dev Aug 22, 2023
14 checks passed
@confluence confluence deleted the pam/1282_sync_id branch August 22, 2023 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
requiring frontend this backend issue requires work in both frontend and backend requiring protobuf
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants