5
\$\begingroup\$

I have never done anything functional before. These two functions are hideous to look at. II think the first step to do is extract the inner bits out and then map over my arrays. What else can I do to make this code a bit more functional?

https://github.com/dbousamra/scalacloth/blob/master/src/cloth/Cloth.scala

Specifically:

  def verletIntegration() = {   

    for (row <- grid) {
      for (p <- row) {
        if (p.stuck) p.setCurrentPos(p.getPreviousPos)

        var multiplyByTime = Tuple2(p.getForces * timestep * timestep, gravity * timestep * timestep)
        var minusPrevPos = Tuple2(p.getCurrentPos.getX - p.getPreviousPos.getX, p.getCurrentPos.getY - p.getPreviousPos.getY)
        var together = Tuple2(multiplyByTime._1 + minusPrevPos._1 , multiplyByTime._2  + minusPrevPos._2)
        p.setPreviousPos(p.getCurrentPos)
        p.setCurrentPos(new Position(p.getCurrentPos.getX + together._1, p.getCurrentPos.getY + together._2))
      }
    }
  }

  def satisfyConstraints() = {
    for (row <- grid) {
      for (p <- row) {
        if (p.stuck) p.setCurrentPos(p.getPreviousPos)
        else {
          var neighbors = p.getNeighbors
          for (constraint <- neighbors) {
            val c2 = grid(constraint.getX)(constraint.getY).getCurrentPos
            val c1 = p.getCurrentPos
            val delta = Tuple2(c2.getX - c1.getX, c2.getY - c1.getY)
            val deltaLength = math.sqrt(math.pow((c2.getX - c1.getX), 2) + math.pow((c2.getY - c1.getY),2))
            val difference = (deltaLength - 1.0f) / deltaLength
            val dtemp = Tuple2(delta._1 * 0.5f * difference, delta._2 * 0.5f * difference)
            p.setCurrentPos(new Position(c1.getX + dtemp._1.floatValue, c1.getY + dtemp._2.floatValue))
            grid(constraint.getX)(constraint.getY).setCurrentPos(new Position(c2.getX - dtemp._1.floatValue, c2.getY - dtemp._2.floatValue))
//          }

          }
        }
      }
    }
  }
\$\endgroup\$
7
\$\begingroup\$
for (row <- grid) {
  for (p <- row) {

//should be written as:

for(row <- grid; p <- row) {


Tuple2("bla", 42)

//should be written as

("bla", 42)

I don't really understand why you use tuples when you have already a class for doing exactly such kind of math, namely Position.

The code could be more functional by using immutable case classes for Positions (but it depends on the problem if this is the right thing to do):

case class Position(x: Float, y: Float) {
    def -(that:Position) = Position(this.x - that.x, this.y - that.y)
    def +(that:Position) = Position(this.x + that.x, this.y + that.y)
}

Then you can write things like pos1 + pos2. Note that you don't need the getter methods, as you can write pos1.x, and no setter methods, as you can write and pos1.copy(x=12). The new isn't required any longer: val p = Position(1, 3).

How to proceed with the refactoring would depend on your decision if you keep Position as it is, or if you want to go with immutable Positions.

[Edit]

Here is how I would refactor it:

Run.scala:

package cloth

object Run {

  def main(args: Array[String]): Unit = {

    import javax.swing.JFrame

    val test = new Screen
    val frame = new JFrame("scalacloth")
    frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE)
    frame.setPreferredSize(new java.awt.Dimension(640, 720))
    frame.getContentPane().add(test)
    test.init
    frame.pack
    frame.setVisible(true)
  }

}

import processing.core._

class Screen extends PApplet {

  val cloth = new Cloth(
    rows = 20, 
    columns = 20, 
    gravity = 0.001f,
    timestep = 0.8f,
    fixedParticles = List(Coordinate(19,0))
  )

  override def setup() {
    cloth.createGrid()
    size(640, 720)
    background(255)
    smooth()
    noStroke()
    fill(0, 102)
  }

  override def draw() {
    background(255)
    fill(255)
    stroke(0)

    def drawLines(particle: Particle):Unit = particle.neighbors.foreach { n =>
      line(particle.currentPos.x * 20 + 20, particle.currentPos.y * 20 + 20,
           cloth.grid(n.x)(n.y).currentPos.x * 20 + 20, cloth.grid(n.x)(n.y).currentPos.y* 20 + 20)
    }

    for(column <- cloth.grid; particle <- column) drawLines(particle)
    cloth.verletIntegration
    cloth.satisfyConstraints
  }
}

Position.scala:

package cloth

case class Position(x: Float, y: Float) {
    def -(that:Position) = Position(this.x - that.x, this.y - that.y)
    def +(that:Position) = Position(this.x + that.x, this.y + that.y)
    def *(scalar:Float) = Position(this.x * scalar, this.y * scalar)
    def length = math.sqrt(x*x + y*y).toFloat
}

Particle.scala:

package cloth

class Particle(
    var currentPos: Position, 
    var previousPos: Position, 
    val gridIndex: Coordinate, 
    val restLength: Float,
    val neighbors: Array[Coordinate],
    var stuck: Boolean 
    ) {

  val forces = 0.0f
}

... and Cloth.scala:

package cloth

import scala.collection

case class Coordinate(x: Int, y: Int)

class Cloth(
  val rows: Int, 
  val columns: Int, 
  var gravity: Float,
  val timestep: Float,
  val fixedParticles: List[Coordinate]
) {

  val grid = new Array[Array[Particle]](rows, columns)

  def createGrid(): Unit = for (x <- 0 until rows; y <- 0 until columns) {
    val coord = Coordinate(x, y)
    val pos = Position(x, y)
    grid(x)(y) = new Particle(pos, pos, coord, 1.0f, findNeighbors(coord),
                              fixedParticles.contains(coord))
  }

  def findNeighbors(coord: Coordinate): Array[Coordinate] = Array(
    coord.copy(x = coord.x - 1),
    coord.copy(y = coord.y - 1),
    coord.copy(x = coord.x + 1),
    coord.copy(y = coord.y + 1)
  ).filter(isOccupied(_))

  def isOccupied(coord: Coordinate): Boolean = 
    (0 <= coord.x && coord.x < rows && 
     0 <= coord.y && coord.y < columns)

  def verletIntegration(): Unit = for (row <- grid; p <- row) 
    p.currentPos = 
      if (p.stuck) p.previousPos else {
        val multiplyByTime = Position(p.forces * timestep * timestep, gravity * timestep * timestep)
        val together = multiplyByTime + p.currentPos - p.previousPos
        p.previousPos = p.currentPos
        p.currentPos + together
    }

  def satisfyConstraints() {

    def calculateConstraint(constraint: Coordinate, p: Particle) {
      val c2 = grid(constraint.x)(constraint.y).currentPos
      val c1 = p.currentPos
      val delta = c2 - c1
      val difference = 0.5f * (1.0f - 1.0f / delta.length)
      val dtemp = delta * difference
      p.currentPos = c1 + dtemp
      grid(constraint.x)(constraint.y).currentPos = c2 - dtemp
    }

    for(row <- grid; p <- row) 
      if (p.stuck) p.currentPos = p.previousPos
      else p.neighbors.foreach(calculateConstraint(_, p))
  }

}

Hope that helps...

\$\endgroup\$
  • \$\begingroup\$ Great post! Yeah I am not sure why I didn't use the position class. I was tired (well thats my excuse :P). \$\endgroup\$ – Dominic Bou-Samra Jul 20 '11 at 7:27
  • \$\begingroup\$ Hmm my scala version borks when i try to not use "new" \$\endgroup\$ – Dominic Bou-Samra Jul 20 '11 at 9:46
  • \$\begingroup\$ That should definitely work: When you write case class, you don't need new. \$\endgroup\$ – Landei Jul 20 '11 at 10:06
  • \$\begingroup\$ @Dominic: You have 2 ways to say 'Great post!', no. 1 is upvoting the answer by hitting the up-arrow ^ at the top of the answer with your mouse, no. 2 is accepting it as the best answer, by marking the check mark. \$\endgroup\$ – user unknown Jul 20 '11 at 16:36
  • \$\begingroup\$ @Dominic Bou-Samra: I updated my answer. Cool application, by the way... \$\endgroup\$ – Landei Jul 20 '11 at 18:04
2
\$\begingroup\$

There is an idea for scala for-loops: You can include the assignments into the production part:

for (constraint <- neighbors) {
        val c2 = grid (constraint.getX) (constraint.getY).getCurrentPos
        val c1 = p.getCurrentPos
        val delta = Tuple2 (c2.getX - c1.getX, c2.getY - c1.getY)
        // ...

Then you don't need the keyword 'val':

for (constraint <- neighbors;
   c2 = grid (constraint.getX) (constraint.getY).getCurrentPos
   c1 = p.getCurrentPos
   delta = Tuple2 (c2.getX - c1.getX, c2.getY - c1.getY)
   // ...

however - I expect a yield, following a for

val x = for (y)
  yield z

And methods which don't take parameters and don't return anything are either useless, or doing sideeffects. Too much vars are the same smell: mutable state.

Try to reach your goal without mutable state, and you will find more functional patterns.

\$\endgroup\$
  • \$\begingroup\$ Hmm yeah these methods are void which is probably not good. \$\endgroup\$ – Dominic Bou-Samra Jul 20 '11 at 5:28
1
\$\begingroup\$

Ooh, Processing with Scala! Good combination...

I took the excellent rewriting of your code by Landei as a base for my own experimentations... I fixed a deprecation warning (Array initialization), made some more variables immutable, improved the Processing side (don't forget: size() must be the first call of setup())... then I went crazy with refactoring, changing names to fit my understanding of the algorithm, etc.

As I am a Scala newbie myself, I found it was a good exercise, but it still has Unit functions...
Maybe you might be interested to see my version of your code:
http://bazaar.launchpad.net/~philho/+junk/Scala/files/head:/_SmallPrograms/ClothSimulation/

\$\endgroup\$
  • \$\begingroup\$ Congratultions! I find it a great way to learn a language, because once you understand the theory, it's not a hard program, but 10x more interesting then other things. Wow your code is a lot nicer then mine, and far simpler. I espeically like how you've simplified the verlet method. \$\endgroup\$ – Dominic Bou-Samra Jul 22 '11 at 22:19
  • \$\begingroup\$ I've started on an implementation of a gameboy emulator in scala. Maybe you want to take a look :P (feel free to commit if you want). It's another good exercises that's hard enough to flex Scala's power (I hope) but easy enough to knock over in a week. \$\endgroup\$ – Dominic Bou-Samra Jul 22 '11 at 22:22

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.