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.

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

Conversation

DimYfantidis
Copy link

C# implementation of Flood fill. Added the code in flood_fill.md

@Amaras
Copy link
Member

Amaras commented Aug 18, 2023

Hello @DimYfantidis and thank you for your interest in the AAA.

We'll try to review your PR as soon as possible, but be aware that it might take a while, as I'm not sure we currently have available C# reviewers.

From what I see, there is a lot of boilerplate code, which seems like a lot to me, but I'm not a C# expert.

I'll nonetheless get a first review path going soon-ish, so that it doesn't get too stuck in review limbo

@Amaras
Copy link
Member

Amaras commented Aug 18, 2023

I tried compiling on my machine, and it simply didn't.
I'm using an Ubuntu 20.04 machine, with the mono-complete package.
Could you provide compilation instructions for Linux?
Here's the output:

❯ mcs FloodFill.cs 
FloodFill.cs(65,33): error CS1525: Unexpected symbol `+', expecting `)', `,', or `identifier'
FloodFill.cs(65,36): error CS1525: Unexpected symbol `)', expecting `)', `,', or `identifier'
FloodFill.cs(66,16): error CS1525: Unexpected symbol `new', expecting `)', `,', or `identifier'
FloodFill.cs(66,34): error CS1525: Unexpected symbol `.', expecting `)', `,', or `identifier'
FloodFill.cs(67,16): error CS1525: Unexpected symbol `new', expecting `)', `,', or `identifier'
FloodFill.cs(67,30): error CS1525: Unexpected symbol `.', expecting `)', `,', or `identifier'
FloodFill.cs(68,16): error CS1525: Unexpected symbol `new', expecting `)', `,', or `identifier'
FloodFill.cs(68,34): error CS1525: Unexpected symbol `.', expecting `)', `,', or `identifier'
FloodFill.cs(156,20): error CS1525: Unexpected symbol `)', expecting `(' or `type'
FloodFill.cs(156,25): error CS1525: Unexpected symbol `0', expecting `(' or `type'
FloodFill.cs(156,28): error CS1525: Unexpected symbol `1', expecting `(' or `type'
FloodFill.cs(156,31): error CS1525: Unexpected symbol `0', expecting `(' or `type'
FloodFill.cs(156,34): error CS1525: Unexpected symbol `0', expecting `(' or `type'
FloodFill.cs(157,16): error CS1525: Unexpected symbol `new', expecting `(' or `type'
FloodFill.cs(157,25): error CS1525: Unexpected symbol `0', expecting `(' or `type'
FloodFill.cs(157,28): error CS1525: Unexpected symbol `1', expecting `(' or `type'
FloodFill.cs(157,31): error CS1525: Unexpected symbol `0', expecting `(' or `type'
FloodFill.cs(157,34): error CS1525: Unexpected symbol `0', expecting `(' or `type'
FloodFill.cs(158,16): error CS1525: Unexpected symbol `new', expecting `(' or `type'
FloodFill.cs(158,25): error CS1525: Unexpected symbol `0', expecting `(' or `type'
FloodFill.cs(158,28): error CS1525: Unexpected symbol `1', expecting `(' or `type'
FloodFill.cs(158,31): error CS1525: Unexpected symbol `0', expecting `(' or `type'
FloodFill.cs(158,34): error CS1525: Unexpected symbol `0', expecting `(' or `type'
FloodFill.cs(159,16): error CS1525: Unexpected symbol `new', expecting `(' or `type'
FloodFill.cs(159,25): error CS1525: Unexpected symbol `0', expecting `(' or `type'
FloodFill.cs(159,28): error CS1525: Unexpected symbol `1', expecting `(' or `type'
FloodFill.cs(159,31): error CS1525: Unexpected symbol `1', expecting `(' or `type'
FloodFill.cs(159,34): error CS1525: Unexpected symbol `1', expecting `(' or `type'
FloodFill.cs(160,16): error CS1525: Unexpected symbol `new', expecting `(' or `type'
FloodFill.cs(160,25): error CS1525: Unexpected symbol `0', expecting `(' or `type'
FloodFill.cs(160,28): error CS1525: Unexpected symbol `0', expecting `(' or `type'
FloodFill.cs(160,31): error CS1525: Unexpected symbol `0', expecting `(' or `type'
FloodFill.cs(160,34): error CS1525: Unexpected symbol `0', expecting `(' or `type'
FloodFill.cs(164,20): error CS1525: Unexpected symbol `)', expecting `(' or `type'
FloodFill.cs(164,25): error CS1525: Unexpected symbol `3', expecting `(' or `type'
FloodFill.cs(164,28): error CS1525: Unexpected symbol `1', expecting `(' or `type'
FloodFill.cs(164,31): error CS1525: Unexpected symbol `0', expecting `(' or `type'
FloodFill.cs(164,34): error CS1525: Unexpected symbol `0', expecting `(' or `type'
FloodFill.cs(165,16): error CS1525: Unexpected symbol `new', expecting `(' or `type'
FloodFill.cs(165,25): error CS1525: Unexpected symbol `3', expecting `(' or `type'
FloodFill.cs(165,28): error CS1525: Unexpected symbol `1', expecting `(' or `type'
FloodFill.cs(165,31): error CS1525: Unexpected symbol `0', expecting `(' or `type'
FloodFill.cs(165,34): error CS1525: Unexpected symbol `0', expecting `(' or `type'
FloodFill.cs(166,16): error CS1525: Unexpected symbol `new', expecting `(' or `type'
FloodFill.cs(166,25): error CS1525: Unexpected symbol `3', expecting `(' or `type'
FloodFill.cs(166,28): error CS1525: Unexpected symbol `1', expecting `(' or `type'
FloodFill.cs(166,31): error CS1525: Unexpected symbol `0', expecting `(' or `type'
FloodFill.cs(166,34): error CS1525: Unexpected symbol `0', expecting `(' or `type'
FloodFill.cs(167,16): error CS1525: Unexpected symbol `new', expecting `(' or `type'
FloodFill.cs(167,25): error CS1525: Unexpected symbol `3', expecting `(' or `type'
FloodFill.cs(167,28): error CS1525: Unexpected symbol `1', expecting `(' or `type'
FloodFill.cs(167,31): error CS1525: Unexpected symbol `1', expecting `(' or `type'
FloodFill.cs(167,34): error CS1525: Unexpected symbol `1', expecting `(' or `type'
FloodFill.cs(168,16): error CS1525: Unexpected symbol `new', expecting `(' or `type'
FloodFill.cs(168,25): error CS1525: Unexpected symbol `3', expecting `(' or `type'
FloodFill.cs(168,28): error CS1525: Unexpected symbol `3', expecting `(' or `type'
FloodFill.cs(168,31): error CS1525: Unexpected symbol `3', expecting `(' or `type'
FloodFill.cs(168,34): error CS1525: Unexpected symbol `3', expecting `(' or `type'
Compilation failed: 58 error(s), 0 warnings

@DimYfantidis
Copy link
Author

Thank you for your time, I'm very sorry but it seems that there are no special compilation instructions for Linux. Normally, "mcs FloodFill.cs" should be enough, but it seems that I've written code that could compile and execute due to specific IDE configurations that I had on my machine. I have made changes to the code and it now compiles successfully on Ubuntu 22.04.3 simply by typing the aforementioned command (It would be very unlucky if it was to fail Ubuntu 20.04 though very unlikely; I believe it should compile fine on your machine now).

I'm not sure on how I could change the code I have commited. Is it possible that, in this specific pull request, I could change the contents of FloodFill.cs? Or should I create a new pull request? If the former is possible, notify me so that we could come in contact on Discord. (I'm a new at contributing to open source projects and github projects)

Thanks again and I'm very sorry.

@Amaras
Copy link
Member

Amaras commented Aug 19, 2023

You can simply modify the code and git add FloodFill.cs and then git commit with a relevant message.
When you then git push, we'll get the up to date code in this PR. :)

@DimYfantidis
Copy link
Author

Done. I'm on standby in case another problem occurs, although now I suppose everything is fine. 👍

@Amaras
Copy link
Member

Amaras commented Aug 20, 2023

Thanks, it compiles now.
I'll try to get a PR going to get C# into our build system to make sure we don't have a problem like that any more.
Hopefully we can get a competent C# developer who can look at your PR soon :)

@Amaras
Copy link
Member

Amaras commented Aug 20, 2023

Alright, the PR to get C# in our build system is up (#1012)

Amaras
Amaras previously requested changes Aug 23, 2023
Copy link
Member

@Amaras Amaras left a comment

Choose a reason for hiding this comment

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

Alright, I'm not a C# expert, so this can only ever be a first pass.

Feel free to disagree with me on everything I said, since I'm not an expert.
The only change I demand is the size computation on each loop that can't be assumed to be optimized away, the rest is up to you (with explanations for disagreements, though, please)

Comment on lines 42 to 49
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

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

@DimYfantidis DimYfantidis Aug 23, 2023

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.

Comment on lines 58 to 61
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

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.


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

@DimYfantidis DimYfantidis Aug 23, 2023

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.

contents/flood_fill/code/csharp/FloodFill.cs Show resolved Hide resolved
Comment on lines 23 to 37
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.

* Removed `Point2I.Equals()`
* Removed `Point2I.GetHashCode()`.
* Optimized/Shortened `FloatListEqualityComparer.Equals()`
* Optimized/Shortened `InBounds()`
* Optimized `FindNeighbors()` (moved `size` out of loop).
* Refactored `flood_fill.md` to fit the changes.
@DimYfantidis
Copy link
Author

Alright, I have refactored code and markdown according to the above discussion.

@Amaras Amaras added Implementation This provides an implementation for an algorithm. (Code and maybe md files are edited.) lang: c# C# programming language labels Aug 23, 2023
@Amaras Amaras dismissed their stale review August 23, 2023 16:14

Changes were made in accordance with my review. I can't approve, but I remove my change requested review

Copy link

@andrewhooker2 andrewhooker2 left a comment

Choose a reason for hiding this comment

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

Feel free to comment back I would love to hear from you!

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

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.

private class Point2I
{
// Coordinates
public int x;

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

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.

}
}

private static bool InBounds(Point2I size, Point2I loc)

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 )


private static bool InBounds(Point2I size, Point2I loc)
{
return loc.x >= 0 && loc.y >= 0 && loc.x < size.x && loc.y < size.y;

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.

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.

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

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

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.


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

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 loc.x >= 0 && loc.y >= 0 && loc.x < size.x && loc.y < size.y;
}

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Implementation This provides an implementation for an algorithm. (Code and maybe md files are edited.) lang: c# C# programming language
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants