sdl2 icon indicating copy to clipboard operation
sdl2 copied to clipboard

createCursor is broken, should pack Bools in Word8, sample working implementation provided

Open Mikolaj opened this issue 3 years ago • 7 comments

https://hackage.haskell.org/package/sdl2-2.5.3.0/docs/SDL-Input-Mouse.html#v:createCursor

is broken, shows every one of four vertical lines. It should probably use

https://hackage.haskell.org/package/base-4.12.0.0/docs/Foreign-C-Types.html#t:CBool

instead of Bool, as per

https://stackoverflow.com/questions/41777675/large-size-of-bool-in-foreign-storable/46784931

However, due to old Ubuntu and so old C sdl2, the latest I can use is sdl2-2.4.1.0, so perhaps this was fixed (no related issue seems to exist, but I didn't search commits). Should I unpack old sdl2, fix it and see if that helps?

Mikolaj avatar Nov 04 '21 15:11 Mikolaj

I've looked at the implementation in my old version:

createCursor dta msk (V2 w h) (P (V2 hx hy)) =
    liftIO . fmap Cursor $
        throwIfNull "SDL.Input.Mouse.createCursor" "SDL_createCursor" $
            V.unsafeWith (V.map (bool 0 1) dta) $ \unsafeDta ->
            V.unsafeWith (V.map (bool 0 1) msk) $ \unsafeMsk ->
                Raw.createCursor unsafeDta unsafeMsk w h hx hy

and CBool would probably not help, because the booleans are converted to Word8. Also, this code is unchanged on master.

Mikolaj avatar Nov 04 '21 21:11 Mikolaj

Oh, the C version says something the Haskell one doesn't: 'The cursor width (w) must be a multiple of 8 bits.' https://wiki.libsdl.org/SDL_CreateCursor

Mikolaj avatar Nov 04 '21 21:11 Mikolaj

Oh, and the data the C function receives clearly needs to be packed, not one bool per Word8.

Mikolaj avatar Nov 04 '21 22:11 Mikolaj

I've fixed this locally: https://github.com/LambdaHack/LambdaHack/commit/b39c31b93dbb974745ee4d3a71a76b5b3f2005b6

Mikolaj avatar Nov 05 '21 00:11 Mikolaj

I guess the decision is what the input type for the correct operation should be. The one I opted for in the implementation is vector of Word8, which gives the most power to the user. However, poweruser can use the Raw versions, so a list of Bools is an option, as well, and I provide a conversion from that to the vector of Word8. Basically, the fix is ready, but requires maintainer's decision.

Mikolaj avatar Nov 09 '21 22:11 Mikolaj

Using bool lists to define cursors produces quite a show for something even remotely large. On the other hand, base-2 literals make it almost nice.

box8x8 :: Vector Word8
box8x8 =
  [ 0b11111111
  , 0b10000001
  , 0b10000001
  , 0b10000001
  , 0b10000001
  , 0b10000001
  , 0b10000001
  , 0b11111111
  ]

Perhaps even better UX would be allowing larger Word sizes:

  [ 0b1111111111111111
  , 0b1000000100000001
  , 0b1000000100000001
  , 0b1000000100000001
  , 0b1000000100000001
  , 0b1000000100000001
  , 0b1000000100000001
  , 0b1111111111111111
  , 0b1000000100000001
  , 0b1000000100000001
  , 0b1000000100000001
  , 0b1000000100000001
  , 0b1000000100000001
  , 0b1000000100000001
  , 0b1111111111111111
  ]

I'm postponing fixing this one in order not to stall next Hackage release. We can bikeshed the options for a while (:

dpwiz avatar Mar 18 '22 21:03 dpwiz

Using bool lists to define cursors produces quite a show for something even remotely large. On the other hand, base-2 literals make it almost nice.

Right. That's not far from the format I'm using in the link above. Newest version: https://github.com/LambdaHack/LambdaHack/blob/7ed94b4b6c75fc46afeef01d8ac476c4598fb822/engine-src/Game/LambdaHack/Client/UI/Frontend/Sdl.hs#L421-L455

Mikolaj avatar Mar 18 '22 21:03 Mikolaj

How about replacing Bool with a FiniteBits a, Storable a polymorphic type? You can then use Word8 .. Word64 to paint cursors in different chunks.

Alternatively, stick to storage type (Word8) and maybe upstream that crosshair-painting function.

dpwiz avatar Sep 02 '22 09:09 dpwiz

Both make sense.

Mikolaj avatar Sep 02 '22 09:09 Mikolaj

I did the second, with a small tweak to charToBool:

    charToBool ' ' = (False, False)  -- transparent
    charToBool '.' = (True, True)  -- visible black
    charToBool _ = (True, False)  -- visible white

dpwiz avatar Sep 02 '22 17:09 dpwiz