Skip to content

Conversation

@c-mateo
Copy link
Contributor

@c-mateo c-mateo commented Apr 13, 2025

Fixes #2504 by restoring part of the code deleted in #2443

Comment on lines +157 to +159
for message in editor.handle_message(BroadcastMessage::TriggerEvent(BroadcastEvent::AnimationFrame)) {
handle.send_frontend_message_to_js(message);
}
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to dispatch both the broadcast event and the increment frame counter message from the CurrentTime input preprocessor message handler. Having these multiple invocations is not pretty. It also somewhat obfuscates that setting the current time now has a side effect (of changing the frame time).
I'm also wondering if it even makes sense to use the animation frame here at all for non render related editor timing. We could consider adding a LogicTick broadcast event which is triggered on a set interval timer and seperate this completely from animation related functionality

Copy link
Member

Choose a reason for hiding this comment

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

I'm also wondering if it even makes sense to use the animation frame here at all for non render related editor timing. We could consider adding a LogicTick broadcast event which is triggered on a set interval timer and seperate this completely from animation related functionality

(To be clear, that would be a separate discussion for a later PR, this one is just focused on un-breaking the functionality without refactoring or rearchitecting it.)

Copy link
Member

Choose a reason for hiding this comment

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

I mean the code should work, I just don't like it, but I didn't like it before either so that is not a merge blocker

@Keavon
Copy link
Member

Keavon commented Apr 13, 2025

!build

@github-actions
Copy link

📦 Build Complete for 8e83678
https://99854ad3.graphite.pages.dev

@Keavon Keavon changed the title Fix auto panning Fix auto-panning that #2443 had broken Apr 13, 2025
@Keavon Keavon changed the title Fix auto-panning that #2443 had broken Restore auto-panning that #2443 had broken Apr 13, 2025
@Keavon Keavon enabled auto-merge (squash) April 13, 2025 22:11
@Keavon Keavon merged commit c2a36ce into GraphiteEditor:master Apr 13, 2025
4 checks passed
@c-mateo c-mateo deleted the 2504-fix-auto-panning branch April 13, 2025 23:32
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.

Auto panning doesn't work (regression)

3 participants