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

Fix examples by resizing the surface #247

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jmjoy
Copy link

@jmjoy jmjoy commented Dec 6, 2024

When running some examples on my Linux system, I encountered an error:

index out of bounds: the len is 0 but the index is 0

This error occurred when attempting to perform array indexing write operations on the buffer generated by surface.buffer_mut().

I successfully resolved these issues by resizing the surface during initialization.

Copy link
Member

@MarijnS95 MarijnS95 left a comment

Choose a reason for hiding this comment

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

This directly conflicts with the idea introduced in #236 (CC @madsmtm) that creating a Surface either infers a size from the given window and/or a Resized event is always delivered somewhere in application startup before the first RedrawRequested event.

Hacking that original misplaced .resize() back in is unfortunately not going to fly. We need to instead debug what your Linux sample is doing, was this on Wayland or X11?

@MarijnS95
Copy link
Member

More context is available in #237, too.

@MarijnS95
Copy link
Member

Analyzing the code, it seems that the Wayland backend would have crashed if resize() is never called, because size contains None. On X11 however, without SHM support (i.e. falling back to sending all pixels over the wire), a default implementation is created with an empty Vec:

Buffer::Wire(Vec::new())

@madsmtm
Copy link
Member

madsmtm commented Dec 6, 2024

Hmm, I'm fine with this PR? It does the same thing as the fruit.rs example does in #236, i.e. resizing the surface upon initialization (since that isn't done yet internally on all platforms).

@MarijnS95
Copy link
Member

Then we should copy-paste that ugly edge-case, specific comment and all, to every other example?

I completely missed that we inconsistently had a weird edge-case in one but not all examples.

@madsmtm
Copy link
Member

madsmtm commented Dec 6, 2024

Then we should copy-paste that ugly edge-case, specific comment and all, to every other example?

I think so, yeah

@madsmtm
Copy link
Member

madsmtm commented Dec 6, 2024

(Or we should fix the underlying issue)

@MarijnS95
Copy link
Member

(Or we should fix the underlying issue)

(Guess why I initially "requested changes" 😉)

@jmjoy
Copy link
Author

jmjoy commented Dec 7, 2024

Hacking that original misplaced .resize() back in is unfortunately not going to fly. We need to instead debug what your Linux sample is doing, was this on Wayland or X11?

X11

@jmjoy
Copy link
Author

jmjoy commented Dec 7, 2024

Hmm, I'm fine with this PR? It does the same thing as the fruit.rs example does in #236, i.e. resizing the surface upon initialization (since that isn't done yet internally on all platforms).

In fact, I modified it based on the example of fruit.rs.

I also think it's ugly to add such a paragraph to each example. It would be better to have one that can fix the underlying issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants