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

The behaviour of runApp with wrong sized canvas is strange #13

Closed
kfl opened this issue Sep 10, 2022 · 2 comments
Closed

The behaviour of runApp with wrong sized canvas is strange #13

kfl opened this issue Sep 10, 2022 · 2 comments
Labels
api change fixing this might result in an API change

Comments

@kfl
Copy link
Member

kfl commented Sep 10, 2022

If the draw argument for runApp returns a canvas that's to small or to big wrt the given arguments, the result is strange.

For instance, the following program:

#r "nuget:DIKU.Canvas, 1.0.0"

let bug =
    Canvas.runApp "Opps" 42 69
                  (fun _ _ _ -> Canvas.create 420 4220) (fun _ _ -> None) ()

Will give an System.ArgumentException: Destination array was not long enough error.

Likewise, the following program:

#r "nuget:DIKU.Canvas, 1.0.0"

let bug =
    let apple = Canvas.fromFile "apple.jpg"
    Canvas.runApp "Oh no" (Canvas.height apple) (Canvas.width apple) // width and height is swapped
                  (fun _ _ _ -> apple) (fun _ _ -> None) ()

Where the width and height arguments to runApp are swapped, results in garbled output.

@kfl kfl added the api change fixing this might result in an API change label Sep 10, 2022
@kfl kfl changed the title The behaviour of runApp with odd sized canvas is strange The behaviour of runApp with odd sized canvas is strange Sep 10, 2022
@kfl kfl changed the title The behaviour of runApp with odd sized canvas is strange The behaviour of runApp with wrong sized canvas is strange Sep 11, 2022
Xatha added a commit to Xatha/diku-canvas that referenced this issue Nov 2, 2022
Xatha added a commit to Xatha/diku-canvas that referenced this issue Nov 2, 2022
@Xatha Xatha mentioned this issue Nov 2, 2022
@Xatha
Copy link
Contributor

Xatha commented Nov 2, 2022

Hi @kfl!

I believe the first instance happens when you try to buffer a frame (canvas), from the draw function, that is larger than the given height h and width w in runApp. The program will crash because our buffer's size is calculated from w, h and not the actual frame's size. Although, this usually does not matter since the frame from draw is usually the same size as w, h. But nothing stops us from creating a draw function that will return a Canvas with a larger (or smaller) size than w, h .

Your example demonstrates this point well. You parse (fun _ _ _ -> Canvas.create 420 4220) to runApp as your draw function. But no matter what w, h is parsed into your expression, you return a Canvas with the dimensions of 420x4220. This is a lot more bytes than the size of the buffer which is initialised for frames of only 42x69. Our buffer is too small to store the data from our frame.

I believe the second instance occurs because swapping the height and width will mess with the pitch calculations for SDL.SDL_UpdateTexture(). Again, this is because we are not basing our calculations on the frames given, but on the given canvas window size.

A possible solution will be to calculate our buffer size from the first frame from draw. Although, this can still theoretically break if draw returns frames that become increasingly larger. Although, I do not believe that would be a correct use of draw.

I attempted to fix this issue, and I created a PR with my proposed solution.

@kfl
Copy link
Member Author

kfl commented Sep 6, 2023

No longer relevant in version 2.0

@kfl kfl closed this as completed Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api change fixing this might result in an API change
Projects
None yet
Development

No branches or pull requests

2 participants