5
\$\begingroup\$

This is a concept for a compositor. It would help me if you could review how well I expressed my ideas into the functional language.

I first divide everything in an areas that each window covers entirely or not:

the idea

Then I can walk the areas and render each corresponding window.

The actual composing of the vectors is a type hole right now because that's not really what this is about.

module SceneGraph where
  import qualified Data.Vector as V
  import Codec.Picture
  import Data.Foldable
  import Data.List
  import Control.Applicative
  import Data.Colour
  import Data.Colour.SRGB
  import Data.Colour.Names

  data Point = Point { xC, yC :: Int}
  --[upleft, downright)! upleft inclusive, downright exclusive
  data Rectangle = Rectangle { upLeft, downRight :: Point}
  --[upleft, downright]! inclusive
  data Area = Area { upLeftA, downRightA :: Point}
  data Window = Window
        { zIndex :: Int,
          area :: Rectangle,
          render :: V.Vector PixelRGB8 -> V.Vector PixelRGB8
        }

  getAllPoints :: [[Window]] -> [Point]
  getAllPoints = foldMap (\z -> [upLeft (area z), downRight(area z)]) . fold

  getXYCoordinatesSorted :: [Point] -> ([Int], [Int])
  getXYCoordinatesSorted = sortPoints . foldr (\z y -> (xC z : fst y, yC z : snd y)) ([], [])

  sortPoints :: ([Int], [Int]) -> ([Int], [Int])
  sortPoints z = ((sort . nub . fst) z, (sort . nub . snd) z)

  getBounds :: ([Int], [Int]) -> ([(Int, Int)], [(Int, Int)])
  getBounds z = ((constructBounds . fst) z, (constructBounds . snd) z)

  constructBounds :: [Int] -> [(Int, Int)]
  constructBounds z = map (\i -> (fst i, snd i - 1)) $ zip z (tail z) ++ [(last z, last z + 1)]

  boundsToAreas :: ([(Int, Int)], [(Int, Int)]) -> [Area]
  boundsToAreas (xAxis, yAxis) = liftA2 boundToArea xAxis yAxis

  boundToArea :: (Int, Int) -> (Int, Int) -> Area
  boundToArea (xUp, xDown) (yLeft, yRight) = Area (Point xUp yLeft) (Point xDown yRight)

  targetedWindows :: [[Window]] -> Point -> [Window]
  targetedWindows windows point = (filter (\z -> isPointInsideRectangle (area z) point) . fold) windows

  isPointInsideRectangle :: Rectangle -> Point -> Bool
  isPointInsideRectangle rect point = (xC point >= xC (upLeft rect) && yC point <= yC (upLeft rect)) && (xC point >= xC (upLeft rect) && yC point <= yC (upLeft rect))

  renderWindows :: [Area] -> [[Window]] -> V.Vector PixelRGB8
  renderWindows areas window = foldr composeRenderings (createBackground (constructBackgroundArea areas)) (map (`renderWindow` window) areas)

  constructBackgroundArea :: [Area] -> Area
  constructBackgroundArea areas = Area ((upLeftA . head) areas) ((downRightA . last) areas)

  renderWindow :: Area -> [[Window]] -> V.Vector PixelRGB8
  renderWindow area windows = foldl (flip render) (createBackground area) (reverse (targetedWindows windows (upLeftA area)))

  createBackground :: Area -> V.Vector PixelRGB8
  createBackground area = _

  composeRenderings :: V.Vector PixelRGB8 -> V.Vector PixelRGB8 -> V.Vector PixelRGB8
  composeRenderings rendering background = _

  lengthArea :: Area -> Int
  lengthArea area = xC (downRightA area) - xC (upLeftA area)

  heightArea :: Area -> Int
  heightArea area = yC (upLeftA area) - yC (downRightA area)

All the functions combined would look like this:

composeWindows :: [[Window]] -> V.Vector PixelRGB8
composeWindows windows = renderWindows ((boundsToAreas . getBounds . getXYCoordinatesSorted . getAllPoints) windows) windows

The distinction between rectangles and areas is pretty ugly, but I don't know if my direction is even sound.

\$\endgroup\$
  • 2
    \$\begingroup\$ Hi! Welcome to Code Review. Good job on your first post! \$\endgroup\$ – TheCoffeeCup Dec 14 '15 at 23:43
2
\$\begingroup\$

There are several ways in which "well expressed into the functional language" can be understood.

Idiom

You make a large use of maps, folds, list, tuples etc. and privilege small functions. That’s idiomatic enough for me.

If you want to write even more idiomatic code, you can use HLint and/or pointfree.io.

For example, pointfree.io suggest:

boundsToAreas (xAxis, yAxis) = liftA2 boundToArea xAxis yAxis

Can be written:

boundsToAreas = uncurry (liftA2 boundToArea)

HLint suggest:

foldr composeRenderings
    (createBackground (constructBackgroundArea areas))
    (map (`renderWindow` window) areas)

Can be written:

foldr (composeRenderings . (`renderWindow` window))
    (createBackground (constructBackgroundArea areas))
    areas

Types

4 types

4 different types are defined in your module (perhaps you put them in this code to make it easier to understand). You should put them in separate modules. It will allow you to create dedicated functions for them.

Confusion

There is a confusion between Area and Rectangle:

  • Rectangle does not include the bottom right point,
  • Area does include the bottom right point.

Therefore, why is the Window’s area a Rectangle ?

A Rectangle and an Area are the same thing, it’s how you use them that makes a difference. How to use them is the functions’ job. You could differentiate them with functions like isInsideInclusive and isInsideExclusive.

Intermediate types

You make use of complex yet generic types like ([Int], [Int]), (Int, Int), [(Int, Int)] or even ([(Int, Int)], [(Int, Int)]) though the functions using them are not generic.

You can use type alias to make their intent clearer. For example:

type VerticalStripe = (Int, Int)
type HorizontalStripe = (Int, Int)

boundToArea :: VerticalStripe -> HorizontalStripe -> Area
boundsToAreas :: ([VerticalStripe], [HorizontalStrip]) -> [Area]

Putting the functions using them into another module would be a good idea.

Readability

You should avoid making too long lines of code and better format your code.

For example, this formatting is hard to read:

isPointInsideRectangle :: Rectangle -> Point -> Bool
isPointInsideRectangle rect point = (xC point >= xC (upLeft rect) && yC point <= yC (upLeft rect)) && (xC point >= xC (upLeft rect) && yC point <= yC (upLeft rect))

While the following formatting is easier to read and tells you something is wrong:

isPointInsideRectangle :: Rectangle -> Point -> Bool
isPointInsideRectangle rect point =
       (xC point >= xC (upLeft rect) && yC point <= yC (upLeft rect))
    && (xC point >= xC (upLeft rect) && yC point <= yC (upLeft rect))

Modularity

In your module, the calling graph is a five levels tree:

enter image description here

For example, you have the following chain:

composeWindows → renderWindows → renderWindow → targetedWindows → isPointInsideRectangle

That’s too much for one module. Plus, what is the direct link between composeWindows and isPointInsideRectangle ?

The composeWindows function makes it clear that it does 2 different things:

composeWindows windows =
    renderWindows (( boundsToAreas
                   . getBounds
                   . getXYCoordinatesSorted
                   . getAllPoints
                   ) windows
                  )
                  windows

In renderWindows first parameter, it calculates the areas. This computation is totally unrelated to a Window or its rendering, this is just a partition of a Rectangle.

Code cleaning

If a function is the unique caller of another function, it can generally include this function into its body via a where statement.

boundsToAreas :: ([(Int, Int)], [(Int, Int)]) -> [Area]
boundsToAreas = uncurry (liftA2 boundToArea)

boundToArea :: (Int, Int) -> (Int, Int) -> Area
boundToArea (xUp, xDown) (yLeft, yRight) =
    Area (Point xUp yLeft) (Point xDown yRight)

can be written:

boundsToAreas :: ([(Int, Int)], [(Int, Int)]) -> [Area]
boundsToAreas = uncurry (liftA2 boundToArea)
    where boundToArea (xUp, xDown) (yLeft, yRight) =
              Area (Point xUp yLeft) (Point xDown yRight)

Doing so make it clear the boundToArea function is not used anywhere else or that it does not need to be exported by your module.

This will also avoid to have all the module functions at the same level which can make the code less readable.

The lengthArea and heightArea functions are not used at all.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.