Flood fill in C# #1011
Flood fill in C# #1011
Conversation
|
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 |
|
I tried compiling on my machine, and it simply didn't. |
|
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. |
|
You can simply modify the code and |
|
Done. I'm on standby in case another problem occurs, although now I suppose everything is fine. 👍 |
|
Thanks, it compiles now. |
|
Alright, the PR to get C# in our build system is up (#1012) |
There was a problem hiding this comment.
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)
| 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); | ||
| } |
There was a problem hiding this comment.
Couldn't you shorten this function like so?
| 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); | |
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
So, I'm afraid the initial function and the suggested changes are not equivalent.
There was a problem hiding this comment.
It was too late, I used the wrong operator.
I meant to use || instead of the first &&
There was a problem hiding this comment.
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.
| if (loc.x < 0 || loc.y < 0) { | ||
| return false; | ||
| } | ||
| return loc.x < size.x && loc.y < size.y; |
There was a problem hiding this comment.
shouldn't this be?
| 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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
presumably, you don't need to recompute size every loop
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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; | ||
| } | ||
| } |
There was a problem hiding this comment.
Is all this necessary, and if so, where?
There was a problem hiding this comment.
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.
|
Alright, I have refactored code and markdown according to the above discussion. |
Amaras
added
Implementation
Amaras
dismissed
their stale review
Changes were made in accordance with my review. I can't approve, but I remove my change requested review
| abstract class FloodFill | ||
| { | ||
| // A simple point on the 2 dimensional integer plane. | ||
| private class Point2I |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
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)) { |
There was a problem hiding this comment.
Would a simple == check not work here?
Just curious if we could do something like
if (oldValue == newValue) return
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I didn't do it for read/write reasons, I did it for passing quickly the data structure into the function in
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
const List<List<float>>& gridin 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.
C# implementation of Flood fill. Added the code in flood_fill.md