All RGB colors in one image

Question

This is a program to solve: Images with all Colors from Code Golf:

Make images where each pixel is a unique color (no color is used twice and no color is missing).

• Create the image purely algorithmically.
• Image must be 256×128 (or grid that can be screenshot and saved at 256×128)
• Use all 15-bit colors (15-bit colors are the 32768 colors that can be made by mixing 32 reds, 32 greens, and 32 blues, all in equidistant steps and equal ranges. Example: in 24 bits images (8 bit per channel), the range per channel is 0..255 (or 0..224), so divide it up into 32 equally spaced shades.)

---

This is my first Scala application, and my first Object Oriented application for a long time.

So, I have a Picture class where you can manipulate(get/set) pixels; and an abstract Painter class which produces a (maybe partial) Picture. And there are different painters inheriting from it for different styles.

Please focus on the application design instead of the algorithm for drawing picture when criticising. I specifically need criticisms for:

• OOP design: Did I use any anti-patterns? How would you model the solution?

• Scala: Is this idiomatic Scala? What is missing?

• Syntax suggestions: It looks like Scala has many syntactic structures and sugars. Is there something which can shorten the code, or make it clearer?

Full code on GitHub, for additional context

import com.sksamuel.scrimage._
import java.awt.image.BufferedImage
import java.awt.image.BufferedImage._
import scala.collection.immutable.Seq
import scala.collection.mutable.Set
import scala.util._

object Main extends App {
  val im1 = new DirectionalPainter( new Picture(256, 128)
                                  , Set(LeftOblique, RightOblique)
                                  ).paint(0.1)
  val im2 = new AvgPainter(im1).paint

  im2.save("/tmp/testOut.png")
}

object Painter {
  def colorSSD(c1: RGBColor, c2: RGBColor): Double = {
    val RGBColor(r1, g1, b1, _) = c1
    val RGBColor(r2, g2, b2, _) = c2
    Math.sqrt(Math.pow(r1-r2, 2) + Math.pow(g1-g2, 2) + Math.pow(b1-b2, 2))
  }
  def coordSSD(c1: (Int, Int), c2: (Int, Int)): Double = {
    val (x1, y1) = c1
    val (x2, y2) = c2
    Math.sqrt(Math.pow(x1-x2, 2) + Math.pow(y1-y2, 2))
  }
  def colorAvg(colors: Seq[RGBColor]): Option[RGBColor] = {
    if(colors.length == 0) None
    else {
      val res = ((0.0, 0.0, 0.0) /: colors){
        case ((r, g, b), RGBColor(r_, g_, b_, _)) => (r+r_, g+g_, b+b_)
      } match {
        case (r, g, b) => {
          val num = colors.length
          RGBColor((r/num).round.toInt, (g/num).round.toInt, (b/num).round.toInt)
        }
      }
      Some(res)
    }
  }
}

case class Coord(val x: Int, val y: Int) {
  def +(other: Coord) = Coord(x+other.x, y+other.y)
  def -(other: Coord) = Coord(x-other.x, y-other.y)
}

abstract class Painter(private val picture: Picture) {
  private val width = picture.width
  private val height = picture.height

  private val possibleColors: Set[RGBColor] = {
    val numColors = (width * height).toDouble
    val depth = Math.pow(numColors, 1.0/3.0).round.toInt
    assert(depth < 256)
    val mul = 256/depth
    val c = for ( r <- Stream.range(0, depth)
                ; g <- Stream.range(0, depth)
                ; b <- Stream.range(0, depth)
              )
            yield RGBColor(r * mul, g * mul, b * mul)
    Set(c:_*)
  }

  private val possibleCoords: Set[Coord] = {
    val c = for ( x <- Stream.range(0, width)
                ; y <- Stream.range(0, height)
                )
            yield Coord(x, y)
    Set(c:_*)
  }

  for(coord <- possibleCoords) {
    picture.get(coord) match {
      case Some(color) => set(coord, color)
      case None => Unit
    }
  }

  assert(possibleCoords.size == possibleColors.size)

  final def set(coord: Coord, color: RGBColor): Boolean = {
    val r = coordEmpty(coord) && colorAvailable(color)
    if(r) {
      this.possibleColors.remove(color)
      this.possibleCoords.remove(coord)
      picture.set(coord, Some(color))
    }

    if(Random.nextDouble() < 0.05) {
      val percent = (1 - (unusedColors.length.toDouble / (width * height) )) * 100
      println(f"$percent%1.2f")
    }

    r
  }

  final def get(coord: Coord): Option[RGBColor] = this.picture.get(coord)

  final def coordValid(coord: Coord): Boolean
    = 0 <= coord.x && coord.x < width && 0 <= coord.y && coord.y < height

  final def coordEmpty(coord: Coord): Boolean
    = possibleCoords.contains(coord)


  final def colorAvailable = possibleColors.contains _

  final def unusedColors: Stream[Color] = possibleColors.toStream
  final def unusedCoords: Stream[Coord] = possibleCoords.toStream

  private final def randomFromSet[a](set: Set[a]): Option[a]
    = if(set.isEmpty) None else Some(set.toVector(Random.nextInt(set.size)))
  final def randomColor: Option[Color] = randomFromSet(possibleColors)
  final def randomCoord: Option[Coord] = randomFromSet(possibleCoords)

  final def neighborsOf(windowSize: Int, coord: Coord): Seq[Coord] = {
    val xs = for { i <- -windowSize to windowSize; j <- -windowSize to windowSize
                 ; if (i != 0 || j != 0) }
             yield Coord(coord.x+i, coord.y+j)
    xs.filter(coordValid)
  }

  final def paint(percentage: Double): Picture = {
    assert(0 <= percentage && percentage <= 1)
    while(unusedColors.size > width * height * (1-percentage)) step
    this.picture
  }

  final def paint: Picture = paint(1)

  def step: Unit
}

class Picture(val width: Int, val height: Int) {
  private val array: Array[Option[RGBColor]] = Array.fill(width * height)(None)
  def get(coord: Coord): Option[RGBColor]
    = this.array(coord.x * height + coord.y)
  def set(coord: Coord, v: Option[RGBColor]): Unit
    = this.array(coord.x * height + coord.y) = v

  def save(path: String): Unit = {
    val im = new Image( new BufferedImage(width, height, TYPE_INT_RGB)
                      , new ImageMetadata(Nil)
                      )
    for(x <- 0 until width)
      for(y <- 0 until height) {
        val color = this.get(Coord(x, y)).getOrElse(Color.White).toPixel
        im.setPixel(x, y, color)
      }
    im.output(path)
  }
}

class LinearPainter(val picture: Picture)
  extends Painter(picture) {
  override def step = {
    val color = unusedColors.minBy(_.toInt);
    val coord = unusedCoords.minBy(coord => (coord.x, coord.y));
    assert(this.set(coord, color))
  }
}

class RandomPainter(val picture: Picture)
  extends Painter(picture) {
  override def step = {
    assert(this.set(randomCoord.get, randomColor.get))
  }
}

class AvgPainter(val picture: Picture)
  extends Painter(picture) {
  override def step = {
    val coords = unusedCoords.map(
      coord => (coord, (neighborsOf(1, coord).map(get)).flatten)
    )
    val coordsSorted = coords.sortBy(- _._2.length)
    val chosenOnes = coordsSorted.takeWhile(_._2.length == coordsSorted.head._2.length)
    for((coord, neighbors) <- chosenOnes) {
      val target = Painter.colorAvg(neighbors).get
      val color = unusedColors.par.minBy(Painter.colorSSD(target, _))
      assert(set(coord, color))
    }
  }
}

abstract class Direction { val unit: Coord }
case object Horizontal   extends Direction { val unit = Coord(1, 0) }
case object Vertical     extends Direction { val unit = Coord(0, 1) }
case object LeftOblique  extends Direction { val unit = Coord(1, 1) }
case object RightOblique extends Direction { val unit = Coord(1, -1) }

class DirectionalPainter(val picture: Picture, val directions: Set[Direction])
  extends Painter(picture) {
  assert(directions.size > 0)

  override def step = {
    val directions_ = directions.toIndexedSeq
    val direction = directions_(Random.nextInt(directions_.length))

    val mid = randomCoord.get
    val bs = Stream.iterate(mid)(_ - direction.unit).takeWhile(coordEmpty _)
    val fs = Stream.iterate(mid)(_ + direction.unit).takeWhile(coordEmpty _).drop(1)
    val coords = bs.reverse #::: fs

    val c = randomColor.get
    val colors = unusedColors.sortBy(Painter.colorSSD(c, _))
    for((coord, color) <- coords.zip(colors)) assert(set(coord, color))
  }
}

By the way, here is the current output if you are interested:

Answer 1

For a first try in Scala, this code is a nice attempt. There are not so many rudiments of imperative style programming, which are usually frequent. It is also quite concise and structured, which produces a good overall impression.

However, there are of course a few things to adjust and to improve. I'll divide them in two categories:

• language issues (programming constructs to use, refactorings or style remarks)

• design issues (related with the structure of the application)

Language Issues

DRY It

The three functions of object Painter contain boilerplate repetitions of instructions like Math.pow(n1 - n2, 2) or (r / num).round.toInt. Dedicated shortcut functions can be created in order to reduce number of arguments and chained calls, for example:

// as private function under Painter
private def squareDiff(v1: Int, v2: Int) = Math.pow(v1 - v2, 2)

// as nested function inside colorAvg
def colorAvgComp(compSum: Double) = (compSum / colors.size).round.toInt

/: vs foldLeft

The /: call in the else block of Painter.colorAvg look really heavy, especially with a single-case matcher. As per Scaladoc, z /: xs is the same as xs foldLeft z. In general, foldLeft is used hugely and easier to read:

// we summarize the RGB components of colors:
val sumColors = colors.foldLeft((0.0, 0.0, 0.0)) {
  case ((accR, accG, accB), color) => (accR + color.red, 
                                       accG + color.green, 
                                       accB + color.blue)
  }

// and return the RGBColor Option using the colorAvgComp shortcut above:
Some(RGBColor(colorAvgComp(sumColors._1), 
              colorAvgComp(sumColors._2), 
              colorAvgComp(sumColors._3)))

In the if condition of the same method, isEmpty is usually a better choice to check if a collection contains something, than length == 0.

for Loops & Comprehensions Consistency

There are weird differences between for loop definitions that are supposed to do similar things. Just be consistent, use either Stream.range or until, but not both. Semicolons are very bizarre at line start and indeed are not necessary.

The for - yeild comprehension produces a collection that can be directly converted using toSet, no need for additional varargs :_* conversions:

// in Painter.possibleColors:
(for { r <- Stream.range(0, depth)
       g <- Stream.range(0, depth)
       b <- Stream.range(0, depth)
    } yield RGBColor(r * mul, g * mul, b * mul)).toSet

Avoid Main Logic in Assertions

assert occurs several times and this is something to appreciate. But the general principle is that assertions are used to validate pre/post-conditions or states, especially in dev phase and can be toggled on/off when necessary. They must not contain calls that change the state or pieces of main logic. If assertions are switched off, the main logic instructions passed to assert will just not be executed and the program will not produce the expected results.

With assertions, the following approach should be used:

val result = myObj.ensureJobExecuted(arg1, arg2)
assert(result)

So in the original code:

// this assertion is OK (but !directions.isEmpty would be more usual)
assert(directions.size > 0)

// this assertion is bad: it changes the state!
assert(set(coord, color)) // FIXME: extract the main logic call outside assert!

Boolean Expressions

These ones occur in the code:

def coordValid(coord: Coord): Boolean = 0 <= coord.x && coord.x < width && 0 <= coord.y && coord.y < height

assert(0 <= percentage && percentage <= 1)

When checking them a bit deeper, I saw that this approach just reproduces some math expressions like 0 < x < 1, but at a first glance with a human eye it might be just confusing. When reading a boolean expression, we are more likely to expect the variable part to the left, like this:

x => 0 && x <= 1
or even more explicit
(x => 0) && (x <= 1)

Which reads as "X is greater than or equal to 0 AND X is less than or equal to 1".

When X is reversed in the two parts, and you begin to read, it can become a brainer uselessly:

0 <= X && X <= 1

"0 is less than or equal to X AND X is less than or equal to 1"

Which approach is better for understanding?

Naming

The function names get and set are simply too vague in Painter. Their current role is access and changing pixel color values, so their names should reflect it: getPixelAt or setColorAt.

step is neither very appropriate. Its role is to choose a pixel and fill it with a calculated color. Shouldn't it be named fillNextPixel instead?

Also avoid duplicate names with underscores like directions and directions_ in DirectionalPainter. First, the underscore is not conventional; second, the purpose of the reference is not clear. Should it be called orderedDirections or indexedDirections or other?

Design Issues

I think that there is a mess about the core part of Painter class, concerning coordinates and colors management, and in set(params) function.

def set

It is intended to set the value of a pixel at a given coordinate, but instead it does too many things:

• changes the possible colors collection

• changes the possible coordinates collection

• outputs the percentage value

• sets the pixel color value (this is what it is intended to do)

Clearly, it is not very friendly with the SRP!

Another question is why does it check if the pixel is already filled and if the color is available? I think that this validation should be produced outside.

Colors and Coordinates

The dedicated vals for possible colors and coordinates look heavy, especially in the context when they are iterated, mapped or sorted on each call in different implementations of step function.

What improvements I can suggest:

1. Painter class will declare colors and coordinates as abstract members, typed as Seq of respective type. The implementors will need to initialize these sequences with the order of items required by the implementor. This will allow to iterate on these collections directly on the abstract layer and make them immutable. For example, LinearPainter will fill the Seqs from min to max; RandomPainter will fill them randomly, etc.

2. Painter.paint function will acquire iterators on colors and coordinates sequences and iterate on their elements until the required percentage is reached.

3. step (named properly) will have a default implementation redirecting to set (also named properly). It will be sufficient for LinearPainter and RandomPainter, but step will need to be overriden in AvgPainter and DirectionalPainter, because they add their specific calculations.

Visibility and Encapsulation

Painter class contains too many public final defs, which is quite suspicious. Number of them should be made private or protected and thus the visual pollution with final will be reduced.