Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Flood fill in C# #1011

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Flood fill in C# #1011

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1cc3119
Flood fill in C#
DimYfantidis Aug 18, 2023
466ad37
Updated code and .md to fit the new lines
DimYfantidis Aug 19, 2023
607e1f5
Refactored Code and Markdown
DimYfantidis Aug 23, 2023
8afaf73
Merge branch 'main' into flood_fill_in_csharp
Amaras Aug 23, 2023
bd16e72
Merge branch 'main' into flood_fill_in_csharp
Amaras Aug 23, 2023
49c96b8
Merge branch 'main' into flood_fill_in_csharp
DimYfantidis Nov 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
190 changes: 190 additions & 0 deletions contents/flood_fill/code/csharp/FloodFill.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,190 @@
using System.Linq;
using System.Diagnostics;
using System.Collections.Generic;


namespace Graphics
{
abstract class FloodFill
{
// A simple point on the 2 dimensional integer plane.
private class Point2I
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi there 👋

Something else that stood out here is that you are using 2I for naming. It might be good to consider renaming this to be 2D since you are dealing with x and y coordinates.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello, thanks for the feedback.

I had initially named it "2D" but then i remembered how OpenGL uses "glVertex2f()" to refer to a vertex (vector) of 2 floats, "glVertex2i" to refer to a vertex of 2 integers and "glVertex2d" to refer to a vertex of 2 doubles. Since we're using points with integers, I used Point2I since according to OpenGL (other libraries do it as well) it would mean "Point of two Integers". I understand that at first glance it's confusing, but now that I have given some context I hope that the logic behind it is clear :)

It's simply a convention that I stick to personally.

{
// Coordinates
public int x;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be a good idea here to consider encapsulating the x and y fields by making them private and exposing them through public properties. This adds a layer of protection and allows for future validation if needed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that according to Object Oriented Programming principles all variables should have setter and getter functions. For that reason, sometimes I don't really like OOP as it takes simple things and makes them complicated (given that Point2I is a simple x,y point class and it's not that much of an interesting object).

The program that I wrote is supposed to be as simple as possible and easy to understand for a beginner programmer and algorithm enthusiast that would like to make a first contact with the flood fill algorithm in C#. I feel that

public int getX() { return x; }
public int getY() { return y; }
public void setX(int x) { this.x = x; }
public void setY(int y) { this.y = y; }

would add extra complexity to the program. Although, no problem from my part, I could implement encapsulation for x and y.

public int y;

public Point2I(int x, int y)
{
this.x = x;
this.y = y;
}

public override bool Equals(object o)
{
if(!(o is Point2I other))
return false;
return this.x.Equals(other.x) && this.y.Equals(other.y);
}

public override int GetHashCode()
{
int hash = 17;
hash = 31 * hash + x;
hash = 31 * hash + y;
return hash;
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is all this necessary, and if so, where?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, it seems that there is no equality comparison nor hashing of Point2I instances in the code. I will remove these functions.


// Utility object for comparing List<List<float>> objects.
private class FloatListEqualityComparer : IEqualityComparer<List<float>>
{
public bool Equals(List<float> one, List<float> two)
{
if (ReferenceEquals(one, two))
return true;
if (one == null || two == null)
return false;
return one.SequenceEqual(two);
}
Copy link
Member

@Amaras Amaras Aug 23, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't you shorten this function like so?

Suggested change
public bool Equals(List<float> one, List<float> two)
{
if (ReferenceEquals(one, two))
return true;
if (one == null || two == null)
return false;
return one.SequenceEqual(two);
}
public bool Equals(List<float> one, List<float> two)
{
return ReferenceEquals(one, two) || one != null && two != null && one.SequenceEqual(two);
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's say one and two are not null. So the expression one != null && two != null evaluates to true. This way,
ReferenceEquals(one, two) && one != null && two != null && one.SequenceEqual(two) can be simplified to
ReferenceEquals(one, two) && one.SequenceEqual(two). If one and two point to different Lists, then the first argument will evaluate to false. That means that, no matter what one.SequenceEqual(two) (if they have equal contents or not) evaluates to, the result will always be false, which ultimately defeats the purpose of overriding of the bool Equals(List<float>, List<float>) function.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I'm afraid the initial function and the suggested changes are not equivalent.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was too late, I used the wrong operator.
I meant to use || instead of the first &&

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes that's better. It didn't cross my mind that you made a typo, I'm sorry.
&& has higher priority than || so I believe you are right.


public int GetHashCode(List<float> list) {
return list.GetHashCode();
}
}

private static bool InBounds(Point2I size, Point2I loc)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing to consider here would be the readability to know what the variables of "siz" and "loc" are trying to convey. It looks as though size is supposed to convey bounds and loc is trying to convey a point.

Variable name change suggestion:

  • siz -> bounds
  • Ioc -> point ( although loc is still a pretty viable name here )

{
if (loc.x < 0 || loc.y < 0) {
return false;
}
return loc.x < size.x && loc.y < size.y;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this be?

Suggested change
if (loc.x < 0 || loc.y < 0) {
return false;
}
return loc.x < size.x && loc.y < size.y;
return loc.x >= 0 && loc.y >= 0 && loc.x < size.x && loc.y < size.y;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, these are equivalent. It is my personal preference to write boolean expressions that aren't too long, although this has no effect on the program's logic. I could shorten the boolean expression just like you suggested.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer the orig if possible

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be preferred to use the original for readability.
If you are wanting to shorten and since there is only two options for return you could use a ternary operator here.

Readability > Complexity for C# Code is what I will always suggest.

}

private static List<Point2I> FindNeighbors(ref List<List<float>> grid, Point2I loc, float oldValue)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this instance I don't think it is necessary for you to use the ref keyword with:
ref List<List<float>> grid

Since you are only reading from the grid and not modifying it, there is no need to use the ref keyword.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't do it for read/write reasons, I did it for passing quickly the data structure into the function in $\mathcal{O}(1)$ time.

From what I've understood from C#, since the ref keyword exists then it's like C++ where everything is passed by value except if you say "pass by reference" explicitly (C++ uses the & symbol). This way, since List<List<float>> is an object of $\mathcal{O}(n^{2})$ space complexity, then copying it should take $\mathcal{O}(n^{2})$ time to pass it inside the function. Passing only its reference should take $\mathcal{O}(1)$ time. In C++ this would be written as

const  List<List<float>>& grid

in order to avoid copying the entire data structure.

Keep in mind that I am using my C++ experience and making some assumptions that the inner mechanisms of C# work in the same way. If what I'm saying is wrong and there is no correspondence between the two languages feel free to correct me.

{
var possibleNeighbors = new List<Point2I>
{
new Point2I(loc.x, loc.y + 1),
new Point2I(loc.x + 1, loc.y),
new Point2I(loc.x, loc.y - 1),
new Point2I(loc.x - 1, loc.y)
};
var neighbors = new List<Point2I>();

foreach (Point2I possibleNeighbor in possibleNeighbors)
{
var size = new Point2I(grid[0].Count, grid.Count);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

presumably, you don't need to recompute size every loop

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case the grid (canvas) was not square but let's say it was:

new List<float>(){0, 0, 1, 0},
new List<float>(){0, 0, 1, 0, 0, 0, 0},
new List<float>(){0, 0, 1, 0, 0, 0},
new List<float>(){8, 0, 1, 1, 1, 1, 1, 1, 1, 1},

This way, the size check is necassary for every loop. Also, in my defense, this is the corresponding for-loop in the C++ implementation:

for (auto const& possible_neighbor : possible_neighbors) {
    const auto size = CartesianIndex{ static_cast<int>(grid[0].size()), static_cast<int>(grid.size()) };
    const auto x = static_cast<std::size_t>(possible_neighbor[0]);
    const auto y = static_cast<std::size_t>(possible_neighbor[1]);
    if (inbounds(size, possible_neighbor) && grid[x][y] == old_value) {
        neighbors.push_back(possible_neighbor);
    }
}

where using CartesianIndex = std::array<int, 2>;. The code above is found in the AAA's implementation of Flood Fill in C++. The C++ implementation is equivalent to mine as the size value is computed in each loop in the exact same way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, the C++ implementation needs an edit.
The thing is, you don't use the size of each line, you use the size of the first line and assume it's the same for each line.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, indeed. I blindly copied and pastied that part from the C++ implementation which is also wrong on this case.
I will probably remove it from the loop, but I could probably come up with a way to find in which line of the matrix the given point is situated, in order to get its length. Thanks for your observations.

var x = possibleNeighbor.x;
var y = possibleNeighbor.y;
if (!InBounds(size, possibleNeighbor)) {
continue;
}
if (grid[x][y].Equals(oldValue)) {
neighbors.Add(possibleNeighbor);
}
}
return neighbors;
}

private static void RecursiveFill(ref List<List<float>> grid, Point2I loc, float oldValue, float newValue)
{
if (oldValue.Equals(newValue)) {
return;
}
grid[loc.x][loc.y] = newValue;

var possibleNeighbors = FindNeighbors(ref grid, loc, oldValue);
foreach (Point2I possibleNeighbor in possibleNeighbors) {
RecursiveFill(ref grid, possibleNeighbor, oldValue, newValue);
}
}

private static void QueueFill(ref List<List<float>> grid, Point2I loc, float oldValue, float newValue)
{
if (oldValue.Equals(newValue)) {
return;
}

var queue = new Queue<Point2I>();
queue.Enqueue(loc);
grid[loc.x][loc.y] = newValue;

while (queue.Count > 0)
{
var currentLoc = queue.Dequeue();
var possibleNeighbors = FindNeighbors(ref grid, currentLoc, oldValue);
foreach (Point2I neighbor in possibleNeighbors)
{
grid[neighbor.x][neighbor.y] = newValue;
queue.Enqueue(neighbor);
}
}
}


private static void StackFill(ref List<List<float>> grid, Point2I loc, float oldValue, float newValue)
{
if (oldValue.Equals(newValue)) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would a simple == check not work here?

Just curious if we could do something like
if (oldValue == newValue) return

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it would, but values are floats and my IDE goes crazy because:
"'==' operator is prone to floating point arithmetic errors. Try using 'abs(oldValue, newValue) < ERROR_CONSTANT' instead"
So, one quick way out is to use the built-in float.Equals(float) method because if C# is smart enough to correct me on it, it should be smart enough to implement it itself.

return;
}

var stack = new Stack<Point2I>();
stack.Push(loc);

while (stack.Count > 0)
{
var currentLoc = stack.Pop();

var x = currentLoc.x;
var y = currentLoc.y;

if (grid[x][y].Equals(oldValue))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest adding a check to see if you are out of bounds here with your x and y.
This would help prevent IndexOutOfRangeException Errors in the future.

{
grid[x][y] = newValue;
var possibleNeighbors = FindNeighbors(ref grid, currentLoc, oldValue);
foreach (Point2I neighbor in possibleNeighbors) {
stack.Push(neighbor);
}
}
}
}


private static void Main(string[] args)
{
// All neighbouring zeros, adjacent to (1, 1), must be replaced with 3 in the end result.
var grid = new List<List<float>>
{
new List<float>(){0, 0, 1, 0, 0},
new List<float>(){0, 0, 1, 0, 0},
new List<float>(){0, 0, 1, 0, 0},
new List<float>(){8, 0, 1, 1, 1},
Amaras marked this conversation as resolved.
Show resolved Hide resolved
new List<float>(){0, 0, 0, 0, 0}
};
var solutionGrid = new List<List<float>>
{
new List<float>(){3, 3, 1, 0, 0},
new List<float>(){3, 3, 1, 0, 0},
new List<float>(){3, 3, 1, 0, 0},
new List<float>(){8, 3, 1, 1, 1},
new List<float>(){3, 3, 3, 3, 3}
};
var startingPoint = new Point2I(1, 1);
var gridComparator = new FloatListEqualityComparer();

var testGrid = new List<List<float>>(grid);
RecursiveFill(ref testGrid, startingPoint, 0, 3);
Debug.Assert(testGrid.SequenceEqual(solutionGrid, gridComparator), "Recursive Flood Fill Failed");

testGrid = new List<List<float>>(grid);
QueueFill(ref testGrid, startingPoint, 0, 3);
Debug.Assert(testGrid.SequenceEqual(solutionGrid, gridComparator), "Queue Flood Fill Failed");

testGrid = new List<List<float>>(grid);
StackFill(ref testGrid, startingPoint, 0, 3);
Debug.Assert(testGrid.SequenceEqual(solutionGrid, gridComparator), "Stack Flood Fill Failed");

}
}
}
10 changes: 10 additions & 0 deletions contents/flood_fill/flood_fill.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,8 @@ In code, this might look like this:
[import:10-25, lang="python"](code/python/flood_fill.py)
{% sample lang="coco" %}
[import:15-20, lang="coconut"](code/coconut/flood_fill.coco)
{% sample lang="cs" %}
[import:64-88, lang="csharp"](code/csharp/FloodFill.cs)
{% endmethod %}


Expand All @@ -118,6 +120,8 @@ In code, it might look like this:
[import:55-63, lang="python"](code/python/flood_fill.py)
{% sample lang="coco" %}
[import:52-61, lang:"coconut"](code/coconut/flood_fill.coco)
{% sample lang="cs" %}
[import:90-101, lang="csharp"](code/csharp/FloodFill.cs)
{% endmethod %}

The above code continues recursing through available neighbors as long as neighbors exist, and this should work so long as we are adding the correct set of neighbors.
Expand All @@ -135,6 +139,8 @@ Additionally, it is possible to do the same type of traversal by managing a stac
[import:27-36, lang="python"](code/python/flood_fill.py)
{% sample lang="coco" %}
[import:23-34, lang:"coconut"](code/coconut/flood_fill.coco)
{% sample lang="cs" %}
[import:126-151, lang="csharp"](code/csharp/FloodFill.cs)
{% endmethod %}

This is ultimately the same method of traversal as before; however, because we are managing our own data structure, there are a few distinct differences:
Expand Down Expand Up @@ -180,6 +186,8 @@ The code would look something like this:
[import:38-53, lang="python"](code/python/flood_fill.py)
{% sample lang="coco" %}
[import:36-49, lang:"coconut"](code/coconut/flood_fill.coco)
{% sample lang="cs" %}
[import:103-123, lang="csharp"](code/csharp/FloodFill.cs)
{% endmethod %}

Now, there is a small trick in this code that must be considered to make sure it runs optimally.
Expand Down Expand Up @@ -264,6 +272,8 @@ After, we will fill in the left-hand side of the array to be all ones by choosin
[import:, lang="python"](code/python/flood_fill.py)
{% sample lang="coco" %}
[import, lang="coconut"](code/coconut/flood_fill.coco)
{% sample lang="cs" %}
[import, lang="csharp"](code/csharp/FloodFill.cs)
{% endmethod %}


Expand Down