Skip to content

Implement revamped RedrawRequested on iOS.#1299

Merged
goddessfreya merged 6 commits intorust-windowing:redraw-requested-2.0from
simlay:redraw-requested-2.0
Dec 22, 2019
Merged

Implement revamped RedrawRequested on iOS.#1299
goddessfreya merged 6 commits intorust-windowing:redraw-requested-2.0from
simlay:redraw-requested-2.0

Conversation

@simlay
Copy link
Copy Markdown
Contributor

@simlay simlay commented Dec 1, 2019

Hi Friends,

Saw the need to help with iOS work for the RedrawRequest 2.0 API in #1082 from @Osspial's request and thought I'd do it.

Honestly, it seemed like too little work but it's quite similar to #1235.

I've tested this with for iced-rs/iced#57 in the ios-examples repo.

  • Tested on all platforms changed
  • Compilation warnings were addressed
  • cargo fmt has been run on this branch
  • cargo doc builds successfully
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented.

@goddessfreya goddessfreya added DS - uikit Affects the UIKit backend (iOS, tvOS, watchOS, visionOS) C - waiting on maintainer A maintainer must review this code labels Dec 1, 2019
@hecrj
Copy link
Copy Markdown
Contributor

hecrj commented Dec 2, 2019

Very happy to see this! 😄

Comment thread src/platform_impl/ios/app_state.rs Outdated
@Osspial
Copy link
Copy Markdown
Contributor

Osspial commented Dec 2, 2019

Thank you so much for taking the time to implement this!

@aleksijuvani You mentioned that you'd be willing to review the iOS RedrawRequested PR. Could you test this out and do a quick review pass over it?

Copy link
Copy Markdown

@ghost ghost left a comment

Choose a reason for hiding this comment

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

I've tested this with a Metal application and I'm not seeing any RedrawEventsCleared events. MainEventsCleared and RedrawRequested are coming in as intended.

Comment thread src/platform_impl/ios/app_state.rs Outdated
@simlay
Copy link
Copy Markdown
Contributor Author

simlay commented Dec 3, 2019

I adjusted the incorrect logic and tested this with the ios-example in the glutin project. It now runs RedrawEventsCleared after MainEventsCleared and after every RedrawRequested.

Comment thread src/platform_impl/ios/app_state.rs Outdated
event: WindowEvent::RedrawRequested,
});
.map(|window| Event::RedrawRequested(RootWindowId(window.into())))
.chain(std::iter::once(Event::RedrawEventsCleared));
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There are two separate paths that can be taken for issuing RedrawRequested events, depending on how the Metal layer was set up. The redraw can either be queued, or it can be issued immediately. See the discussion here.

Right now this is only taking the queued redraws into account. That is to say, if a redraw event is emitted from draw_rect (this is invoked by setNeedsDisplay), RedrawEventsCleared and RedrawRequested will be emitted in the wrong order.

Perhaps we can change request_redraw to always call queue_gl_or_metal_redraw and get rid of the draw_rect code path. This would ensure that the events are issued in the correct order.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah. Good catch. I was unaware of the setNeedsDisplay path. I'm a little apprehensive to remove the draw_rect codepath because it would prevent using winit for a UIKit backed app if I understand correctly. That being said, I'm not a huge fan of duplicating the lines to the draw_rect logic but eh.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In any case, I've added the RedrawRequested and RedrawEventsCleared the draw_rect function as seen below. That said, I don't quite understand how RedrawEventsCleared and RedrawRequested would have been emitted in the wrong order rather than RedrawEventsCleared not being emitted previously. I'll look into testing this.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

That said, I don't quite understand how RedrawEventsCleared and RedrawRequested would have been emitted in the wrong order rather than RedrawEventsCleared not being emitted previously. I'll look into testing this.

My understanding is that there would be no queued RedrawRequested events so the iterator is empty—the empty iterator is then chained with a std::iter::once(RedrawEventsCleared), so a RedrawEventsCleared is always emitted in that code path. The actual RedrawEvents is emitted from the other code path at a different time, hence the events appearing out of order.

I'm a little apprehensive to remove the draw_rect codepath because it would prevent using winit for a UIKit backed app if I understand correctly.

In practice this would mean that you would no longer receive RedrawRequested events from the system and would have to issue them yourself. I'm not sure if you were receiving these events from the system in the first place however. The rules for when drawRect gets called are a bit complicated and depends on how things were set up w.r.t. layers and such.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

As for testing this, I believe you can invoke the drawRect code path with gfx-hal examples. If the Metal layer is a sublayer of the winit created layer (which is what gfx-hal does on iOS), you'll be able to call request_redraw and invoke the drawRect code path.

Taking a quick glance at the new changes—I think this would result in RedrawEventsCleared being emitted, followed by RedrawRequested events, and then another RedrawEventsCleared. We would essentially be receiving duplicate RedrawEventsCleared events because both code paths emit it (provided that draw_rect is invoked that is).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Taking a quick glance at the new changes—I think this would result in RedrawEventsCleared being emitted, followed by RedrawRequested events, and then another RedrawEventsCleared. We would essentially be receiving duplicate RedrawEventsCleared events because both code paths emit it (provided that draw_rect is invoked that is).

I was afraid that would be the case. I think it was worse than that actually. The chain operator added RedrawEventsCleared to every call for handle_main_events_cleared even without a RedrawRequested event the .main_events_cleared_transition function returned nothing.

Anyway, I've updated that issue and tested the Metal backend codepath using the gluten iOS example.

For the UIKit backend, I tested out the events using a some UIKit bindings here https://github.com/simlay/rust-uikit/blob/add-winit/ios-example/rust/src/lib.rs#L75. I found a few issues with some of the logs warning with something similar to the following:

processing non `RedrawRequested` event after the main event loop: RedrawEventsCleared

My guess as to why this is happening is because of the objective-c runtime is asynchronous so, I'm guessing this is suppose to be the behavior. Aside from that, the events came in order and weren't duplicated.

Copy link
Copy Markdown
Contributor

@goddessfreya goddessfreya left a comment

Choose a reason for hiding this comment

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

Thanks for the review everybody, and thanks for the pr @simlay. Merging!

@goddessfreya goddessfreya merged commit c352ad7 into rust-windowing:redraw-requested-2.0 Dec 22, 2019
Osspial pushed a commit that referenced this pull request Dec 22, 2019
* Implement revamped `RedrawRequested` on iOS

* Added RedrawEventsCleared to events_cleared logic

* Fixed from comments

* Added RedrawEventsCleared to draw_rect handler.

* Fixed out of order `RedrawEventsCleared` events.

* cargo fmt
Osspial pushed a commit that referenced this pull request Dec 22, 2019
* Implement revamped `RedrawRequested` on iOS

* Added RedrawEventsCleared to events_cleared logic

* Fixed from comments

* Added RedrawEventsCleared to draw_rect handler.

* Fixed out of order `RedrawEventsCleared` events.

* cargo fmt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C - waiting on maintainer A maintainer must review this code DS - uikit Affects the UIKit backend (iOS, tvOS, watchOS, visionOS)

Development

Successfully merging this pull request may close these issues.

4 participants