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

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 all 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
168 changes: 168 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,168 @@
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

@DimYfantidis DimYfantidis Nov 11, 2023 •
edited
Loading

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;
}
}

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

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 )

{
return loc.x >= 0 && loc.y >= 0 && loc.x < size.x && loc.y < size.y;
Copy link

Choose a reason for hiding this comment

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

With your functions I notice that you don't handle for errors:
A simple check you could consider throwing in would be something like:

if (bounds == null || point == null) throw new ArgumentNullException(bounds == null ? nameof(bounds) : nameof(point));
Feel free to update this to fit your needs.

Copy link

Choose a reason for hiding this comment

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

I won't add in this comment per function but, it is something to consider.

}

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>();
var size = new Point2I(grid[0].Count, grid.Count);

foreach (Point2I possibleNeighbor in possibleNeighbors)
{
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:43-67, 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:69-80, 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:104-129, 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:82-102, 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