phyrondev / openimageio

High Level Rust Wrapper for OpenImageIO
6 stars 0 forks source link

Region of Interest -- Better Naming #3

Open virtualritz opened 2 weeks ago

virtualritz commented 2 weeks ago

The term "region of interest" is not really used outside of VFX (medical imaging seems to be the exception). And even in VFX its use is arguably limited:

ROI is notably Return of Investment everywhere else. What is most confusing to new developers that even know the term is that they haven't come accross its abbreviation. I.e. DaVinci and Ae users know "Region of Interest" but they never heard "ROI".

It is unclear why we need "of interest". The latter is very much implied by what is being done in the code. "Render Region" seems much clearer in conveying what this is used for.

I guess I'm saying something like ImageBuf::fill() is not called fill_operation() or fill_pixels_with_color() since some things can be assumed. Just as it can be assumed that we're really interested in the region we specify an operation to restrict it's workings to. :wink:

Other terms computer graphics often uses are "bounding box" or the shorter "bounds".

But then we have yet another name, OpenEXR uses "window" for regions, namely "display window" and "data window".

And confusingly, for the unititated user, OIIO uses these terms too, in the docs but with the term "pixel" prepended as in "pixel display window" and "pixel data window".

Speaking of regions with specific names & meanings: OIIO uses the term "full" to mean "display window". ImageBuf::set_full() is probably the worst method name in the whole API. Slightly better names would be set_roi_full() or set_full_roi().

But they're still pretty bad because the display window this sets does not neccessaritly show the full image, it could be a crop or be completely outside the data window even.

Also to consider is the concept of an unspecified region that encompasses the whole image. I.e. the assumed union of display- and data windows. It's obtained via ROI::All() which is modeled in (C++) by setting the struct's x_min to INT_MIN -- possibly because of the lack of sum types.

On the Rust side we need to decide two things:

  1. Do we replicate this very confusing use of terms that all refer to essentially rectangular regions? Namely in OIIO: ROI, full, window.

    ROI in OIIO is also used to e.g. create an ImageBuf of certain dimensions. In that case it's not a ROI any more but more just "rectangle"?

    Or do we use one term everywhere? This should IMHO be one of Region, Rect/Rectangle, Bounds, BoundingBox or Window.

    Region could be ok compromise because of the current name, "region of interest". And Region is only 3 letters longer than ROI. For the sake of point 2. being easier on the eyes I assume we will use Region there.

    Bounds is the same length as Region and closer to the common term "bounding box".

    BoundingBox is pretty clear, the longest and some may argue that a typical bounding box isn't axis-aligned or else it's called an AABB (axis-aligned bounding box).

    Window is possibly nice for people acquainted w. OpenEXR metadata but it is confusing to everyone else because the term is more commonly used within a GUI context. I would avoid the term completely and just mention that OpenEXR does use it and how it maps to OIIO, in the docs.

  2. Do we use a custom enum to represent a ROI?

    What then about the variant that describes an actual region? It can't have the same name as the enum.

    1. Assuminhg we use the term Region for the enum:

      enum Region {
        #[default]
        All,
        Bounded(Bounds),
      }
      
      // The following is binary-compatible with the C++ `ROI` struct.
      #[repr(C)]
      struct Bounds {
        x: Range<i32>,
        y: Range<i32>,
        z: Range<i32>,
        channel: Range<u32>,
      }

      This means we never have Option<&Region> but always &Region as parameters since the default is Region::All. This makes for more readable code at call sites:

      /// Set display window to match data window.
      my_image_buffer.set_display_region(&Region::All);
    2. Or do we use an Option<&Region> where None means "all"?

      #[repr(C)]
      struct Region {
        x: Range<i32>,
        y: Range<i32>,
        z: Range<i32>,
        channel: Range<u32>,
      }

      That would make code less readable at call sites since it is unclear what None specifies. However, most modern editors/IDEs will display parameter names as part of inlay hints.

      /// Set display window to match data window.
      my_image_buffer.set_display_region(None);

      I.e. inside a modern IDE w. inlay hints:

      my_image_buffer.set_display_region(region: None);
    3. A third option is to go with the first variant and impl From<&Region> for Option<&Bounds> which basically means Region::All would easily be cast into() then yielding an Option<&Bounds>::None which allows for very Rust-like handling of this case in the code.


TLDR; Option<&Region> feels to me like: