[Feat]: Image loading using ocaml-imagelib library by pawaskar-shreya · Pull Request #113 · claudiusFX/Claudius

pawaskar-shreya · 2025-08-22T00:10:34Z

Helloww @mdales 👋👋

Here's PR for the image loading module

In this PR, I have added the picture module which loads PNG image using ocaml-imagelib.
I have also added picture type t, but it is not directly made available for public use, abstractions have been put up in place to make sure that the original image data isn't tampered
Initially I was taking width and height both as parameters from the user, but then after some testing I saw the aspect ratio get distorted. So now we only take scale from the user and the corresponding dimensions are then caldulated.
Also, both the original and scaled dimensions are made available
I have taken care of a couple of things in this PR, scaling for eg is kept to be float instead of int, to make sure the scaling is smooth and not like a step function
The offsets when positive shift the img to the right, but negative offsets have been taken care to shift to the left
If the image happens to be greater than the sdl window, it is not stretched or fitted into the sdl window. but is sort of rendered as much as possible with changing the dimensions of the screen
A draw_picture function has been introduced in the framebuffer.ml
Picture is also now added as a primitive

I have also written a test_picture module but havent added it here, as it is using an image, so I would like to have your opinion here on how to proceed.

pawaskar-shreya · 2025-08-22T00:25:21Z

Ohh this is a strange error for the build checks

Run opam exec -- dune build
File "src/dune", line 10, characters 40-53:
10 |  (libraries tsdl giflib crunch imagelib imagelib.unix))
                                             ^^^^^^^^^^^^^
Error: Library "imagelib.unix" not found.
-> required by library "claudius" in _build/default/src
-> required by _build/default/META.claudius
-> required by alias all
-> required by alias default
(cd _build/default/src && /home/runner/work/Claudius/Claudius/_opam/bin/ocaml-crunch -m plain -o builtins.ml ../fonts)
Generating builtins.ml
Skipping generation of .mli
Error: Process completed with exit code 1.

this is working locally for me but maybe that can also be attributed to the fact that I had vendored imagelib. Is there any way that you think we can make this work on CI?

mdales · 2025-08-22T08:01:10Z

this is working locally for me but maybe that can also be attributed to the fact that I had vendored imagelib. Is there any way that you think we can make this work on CI?

Yes - how do you think CI knows about the other things Claudius depends on @pawaskar-shreya? If you can find that then you can solve this :)

mdales

Overall, loving this PR: good decision to bring in Imagelib to do the image decoding here rather than just relying on GIFLib, you've added images to the right places (as a primitive and as a draw function on frame buffer).

I have two things that I think you should explore here:

Firstly the scaling confused me in the API - I think the scaling is a good idea, and encourage you not to lose it, but should it be on Picture.t or should it be an argument to the drawing function? When I read the original API, my assumption was that having provided the scaling then when I call pixels I'd get the image already scaled. But that isn't the case, the scaling is only used by the drawing routine - on which case, perhaps we should just pass the scaling there as an argument? That would then make it easier to implement something like this: https://www.youtube.com/watch?v=_P00jYB81ZQ without having to create many copies of the Picture.

Secondly, and more importantly, this abstraction doesn't work if I load two or more images, because the way the palette system works. All the images will have a palette that starts at 0 and works up, and whilst I can change the palette, if I try to draw two images the one that is used second will have the winning palette, making the first image look odd.

I think given this limitation of Claudius is might be best to take image loading away from the user, and have Claudius do it, in a sort of similar way to how I built up a palette programmatically in my FunOCaml talk (https://github.com/mdales/funocaml24/blob/main/bin/main.ml#L146). It could perhaps work like this:

Screen.create would take an optional list of image filenames, along with the user provided palette
Inside Screen.create we load the images into a list of Picture.t, and each image in turn will get an offset to add to its palette definition
The palette for Claudius is then a concatenation of the user provided palette (which might be empty if they just want to use the image's palette) concatenated with all the image palettes in turn.
Screen can then have a pictures property of type Picture.t array that returns the images in the same order that the file names were provided.

This means the user never has to do any palette manipulation by default (which I think new users won't want to worry about), and you can ensure that all loaded images have their own unique set of colours.

We already do this for fonts if you don't want to use the built in system font: you provide it to screen, and then when you want it later you call Screen.font. This also promotes lack of global variables which is a good thing in programming, and particularly in functional programming as global variables are effectively side effects of sorts.

How does this sound?

I know that's a lot of feedback, but overall I'm super happy with this technically, I just think the APIs need considering a little more.

mdales · 2025-08-22T08:08:00Z

+type t
+(** Abstract type representing a loaded picture *)
+
+val load : string -> float -> t


A couple of thoughts here:

Could we call this from_file just to keep it consistent with giflib?

I think the common case will be that most people leave the scale as 1.0, and so I'd advocate that the scale parameter is an optional parameter, and by default we don't do any scaling.

I'd also suggest the filename argument should be last so as to make it easier to do things like:

let img = get_user_input_of_filename () |> Picture.load

mdales · 2025-08-22T08:09:03Z

+val scaled_width : t -> int
+(** [scaled_width pic] returns the width in pixels after applying the scale. *)
+
+val scaled_height : t -> int


For both this and the original sizes can we just have a single method that returns a tuple containing width and height? That then is consistent with Screen.dimensions.

mdales · 2025-08-22T08:09:50Z

+(** [set_scale pic s] returns a new picture with the scale factor updated to
+    [s]. *)
+
+val scaled_width : t -> int


I think this should not need the scaled_ prefix - you set the scaling at load time, and then that's it, that's your image.

mdales · 2025-08-22T08:11:27Z

+val original_width : t -> int
+(** [original_width pic] returns the original width in pixels. *)
+
+val original_height : t -> int


I understand why you added this, but when designing an API one should keep it as small as possible, as then there is less to test, less to maintain, etc.

What use is the original size to the user? surely they only need to know the size it will be drawn at?

mdales · 2025-08-22T08:13:36Z

 (** [filled_polygon points colour framebuffer] Draws a filled polygon made from
    the list of [points] in the specified [colour] into [framebuffer]. *)

+val draw_picture : Picture.t -> int -> int -> t -> unit


draw_char and draw_string take the position as the first arguments - for consistency this should match those (I don't think they're better than what you've done - just we should be consistent).

mdales · 2025-08-22T08:22:51Z

+  in
+
+  let palette_rgb_24 =
+    0x000000


Maybe add a comment about why you're doing this.

Can we avoid this by using a sentinel value (say -1) in the pixel array rather than 0?

I still think this isn't needed: If we have an invalid value rather than zero to represent transparent in the pixel array.

mdales · 2025-08-22T08:24:49Z

+      let idx = (src_y * src_w) + src_x in
+      let color_index = pixels.(idx) in
+
+      if color_index <> 0 then


Perhaps a comment here as to why 0 is being special cased here

mdales · 2025-08-22T09:13:37Z

I had a brief panic that we would not be able to use Imagelib, as the opam info says it is licensed under GPL-3.0, and that is not compatible with the ICS/MIT style license Claudius uses.

However, when I go to the project page, it is show as using the LGPL, which is okay:

https://github.com/rlepigre/ocaml-imagelib/blob/master/LICENSE

pawaskar-shreya · 2025-08-25T06:00:21Z

Heyy @mdales

the builds are again failing, this time I have added imagelib and iamgelib.unix to both dune-project and opam. it just says dune not found ☹️

I don't understand why this is the case. do I need to raise a seperate PR for these: imagelib and iamgelib.unix ; as even if these changes are added in this PR only, CI still follows the old CI workflow. But does that also seems contradicting as the dependencies are anyways fetched from opam file.

Run opam exec -- dune build
  opam exec -- dune build
  shell: /usr/bin/bash -e {0}
  env:
    OPAMCOLOR: always
    OPAMCONFIRMLEVEL: unsafe-yes
    OPAMDOWNLOADJOBS: 4
    OPAMERRLOGLEN: 0
    OPAMEXTERNALSOLVER: builtin-0install
    OPAMPRECISETRACKING: 1
    OPAMRETRIES: 10
    OPAMROOT: /home/runner/.opam
    OPAMSOLVERTIMEOUT: 600
    OPAMYES: 1
    CLICOLOR_FORCE: 1
[ERROR] Command not found 'dune'

does explicitly adding a dune install stanza in main.yml sound like a good option here?

mdales · 2025-08-25T06:08:56Z

@pawaskar-shreya I don't understand why main builds and your PR does not, you've not done anything I can see to cause dune to fail. That said, we don't have dune as a dependancy. Can you add a stage to the CI script main.yml like so and see if this fixes it?

...
      - name: Install system requirements
        run: sudo apt install -y libsdl2-dev

      - name: Install dune
        run: opam install dune

      - name: Install ocaml requirements
        run: opam install . --deps-only --with-test --with-dev-setup --with-doc
...

pawaskar-shreya · 2025-08-25T06:18:07Z

That said, we don't have dune as a dependancy

dune is present as a dependency in both dune-project and claudius.opam. So my thought was that it was fetched initially from there only in all our previous PRs. Dont know why it is not doing the same now. ☹️

Can you add a stage to the CI script main.yml like so and see if this fixes it?

Yupp! raising a PR!

mdales

Thanks for working with my requests! I think that change makes things more easier to use for new users and the common use case.

The one thing I think that isn't done in an idiomatic functional way is that you've added some global state for building up the palettes. You can do this without introducing global state I think if in the Screen.create function you replace the list.map with a list.fold of some sorts and as you load each image you work out its offset and then pass that as an optional argument to the next image load.

Otherwise I think the other small things are:

You still have save separate width/height getters for the image rather than just a single call to get both
No tests
Still using a 0 for the sentinel transparent value, where as I think we should avoid using valid colours

pawaskar-shreya · 2025-08-26T11:48:46Z

The one thing I think that isn't done in an idiomatic functional way is that you've added some global state for building up the palettes. You can do this without introducing global state I think if in the Screen.create function you replace the list.map with a list.fold of some sorts and as you load each image you work out its offset and then pass that as an optional argument to the next image load.

Okayy yea, I should try this!

Otherwise I think the other small things are:

You still have save separate width/height getters for the image rather than just a single call to get both
No tests
Still using a 0 for the sentinel transparent value, where as I think we should avoid using valid colours

Yupp! I am yet to do a bit work here as right now the pixel bleeding problem is also sort of in my way. But yea, once that is done, I shall get these changes in too!

pawaskar-shreya · 2025-08-27T19:37:43Z

Heyy @mdales

I have made some of the changes:

I eliminated the use of global state
And I have added tests for picture module
I also did the change you suggested for test dune for having the assets available in builds. We've got it working! Thankssss! 🥳
There are some changes yet to be made, but lets get them in once we are done with our talk!

But please do suggest me and give your feedback on the recent commits!

mdales

Nicely done with the no globals!

I've marked this as approved, as we can merge this, however it would be nice if time allows to review the use of the 0 element in each image palette - but I can always fix that afterwards, because this already makes Claudius better, so I'm happy for it to merge if you need to focus on your presentation.

mdales · 2025-08-28T05:55:34Z

 (** [updated_entry pal index new_color] checks for the index then returns a new
    palette with the entry at [index] updated to [new_color]. *)
+
+val concat : t list -> t


Ideally we'd have something in test_palette.ml corresponding to this.

mdales · 2025-08-28T05:56:40Z

+  in
+
+  let palette_rgb_24 =
+    0x000000


I still think this isn't needed: If we have an invalid value rather than zero to represent transparent in the pixel array.

mdales · 2025-08-28T05:59:18Z

+
+type t = { palette : Palette.t; pixels : int array; width : int; height : int }
+
+let load_png_as_indexed (filepath : string) : Palette.t * int array * int * int


Whilst you're only testing it for PNG, doesn't using ImageLib here mean you can load any image that ImageLib supports? Nothing here seems PNG specific. Can this just become load_as_indexed?

mdales · 2025-08-28T06:06:03Z

+        pixel_write fb_x fb_y color_index fb
+    done
+  done;
+  fb.dirty <- true


This isn't needed now as you call pixel_write, which itself will set the dirty flag. Not bad, just less code is always good :)

mdales · 2025-08-28T06:20:52Z

  | FilledTriangle of point * point * point * int
  | Char of point * Font.t * char * int
  | String of point * Font.t * string * int
+  | Picture of point * Picture.t


This ideally would have a scale on it - I realised I couldn't use the primitives for the flying ocaml logo example because there is no scale argument here.

mdales requested changes Aug 22, 2025

View reviewed changes

pawaskar-shreya force-pushed the feat/picture-loading branch from a5063b2 to c4af62b Compare August 25, 2025 05:43

pawaskar-shreya force-pushed the feat/picture-loading branch from c4af62b to 2faaed8 Compare August 26, 2025 09:43

mdales requested changes Aug 26, 2025

View reviewed changes

pawaskar-shreya added 19 commits August 27, 2025 19:30

core picture module added

4d68ca8

documentation for picture module

0632128

draw_picture added to framebuffer

bb01b5a

documentation for draw_picture

6d7d354

added picture as a primitive

4974885

src/dune changes added

b68d073

updated dune-project to include imagelib

b7e2b9b

added ensure_palette_offset and with_palette_offset

4860c8e

using ensure_palette_offset in from_picutre

70e0c90

added concat for palette concatenation

9f14347

managing palette offsets and image loading

ae077d4

updated all the documentation

836d05f

removed imagelib.unix from dune-project and opam

3f6b597

ran formatting

41f3533

replacing the duplicate code with pixel_write

7f9b297

repalcing the duplicate code with pixel_write

a182a2b

removed the unnecessary check

acb9a2f

removed the global state

ba07cec

cleaning of the api

6215794

pawaskar-shreya added 3 commits August 27, 2025 19:30

ran formatting

e1e1449

tests added for the picture loading module

056bb50

ran formatting

e5fd6b6

pawaskar-shreya force-pushed the feat/picture-loading branch from b6a2031 to e5fd6b6 Compare August 27, 2025 19:38

mdales approved these changes Aug 28, 2025

View reviewed changes

mdales reviewed Aug 28, 2025

View reviewed changes

mdales mentioned this pull request Aug 28, 2025

Add flying camels example claudiusFX/claudius-examples#18

Merged

pawaskar-shreya merged commit 77a6f75 into claudiusFX:main Aug 28, 2025
1 check passed


		type t = { palette : Palette.t; pixels : int array; width : int; height : int }

		let load_png_as_indexed (filepath : string) : Palette.t * int array * int * int

Conversation

pawaskar-shreya commented Aug 22, 2025

Uh oh!

pawaskar-shreya commented Aug 22, 2025

Uh oh!

mdales commented Aug 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdales left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mdales commented Aug 22, 2025

Uh oh!

pawaskar-shreya commented Aug 25, 2025

Uh oh!

mdales commented Aug 25, 2025

Uh oh!

pawaskar-shreya commented Aug 25, 2025

Uh oh!

mdales left a comment

Choose a reason for hiding this comment

Uh oh!

pawaskar-shreya commented Aug 26, 2025

Uh oh!

pawaskar-shreya commented Aug 27, 2025

Uh oh!

mdales left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mdales commented Aug 22, 2025 •

edited

Loading