Skip to content

Conversation

@robin-condition
Copy link

Gravity, Velocity, Position, DrawColor, more for individual points.
Working on a unified PhysicsObject class to treat many points as one object.

Gravity, Velocity, Position, DrawColor, more for individual points.
Working on a unified PhysicsObject class to treat many points as one object.
return val;
}
public static float ClampMin(float min, float val)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Per discussion with Peter, the idea is a one-side capping of the value, rather than automatic return of a max value, as Math.Max would do.

}
return val;
}
public static float ClampMax(float max, float val)
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Per discussion with Peter, the idea is a one-side capping of the value, rather than automatic return of a min value, as Math.Min would do.

Copy link
Contributor

@glen3b glen3b left a comment

Choose a reason for hiding this comment

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

I haven't looked too deeply at the actual physics implementation, but I've briefly reviewed the general API and I have a handful of preliminary pointers.

{
public static class PhysicsExtensions
{
public static bool ContainsPoint(this List<PhysicsPoint> points, Point point, bool mustInteractWithEnvironment = true)
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like it could take an IEnumerable<PhysicsPoint> instead of a list, is there any particular reason you need it to be ordered?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, IEnumerable is a better choice here.

return false;
}

public static Point BottomLeft(this List<PhysicsPoint> points)
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider whether this can take an IEnumerable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, IEnumerable is a better choice here.


namespace ConsoleGameLib
{
public class PhysicsPoint
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider overloading Equals, GetHashCode, and the corresponding operators.




public List<PhysicsPoint> Contents
Copy link
Contributor

Choose a reason for hiding this comment

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


Microsoft Visual Studio Solution File, Format Version 11.00
# Visual C# Express 2010
Microsoft Visual Studio Solution File, Format Version 12.00
Copy link
Contributor

Choose a reason for hiding this comment

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

This will break compatibility with developing in VS express 2010, this is probably OK but is worth evaluating.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok with that. VS2010 is a discontinued product, it can't even be installed (or more precisely, activated once installed) anymore.

{

static void Main(string[] args)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Please comment your example code extensively so it is easy to tell what it does without referring to other code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Comments are good. I agree with this suggestion

{
foreach(PhysicsPoint entry in points)
{
if(entry.Position.X == point.X && entry.Position.Y == point.Y && (mustInteractWithEnvironment == true ? entry.InteractsWithEnvironment : true))
Copy link
Contributor

Choose a reason for hiding this comment

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

entry.Position is of type ConsoleGameLib.CoreTypes.Point, which currently does not have equality operators overloaded. I plan to submit a separate pull request which fixes this. If that goes through, I suggest using overloaded equality operators.

In addition, it appears mustInteractWithEnvironment is a boolean. If this is the case, you can eliminate the == true (minor code style comment).

Copy link
Contributor

Choose a reason for hiding this comment

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

Great suggestions, agreed. Will review your PR shortly.

Copy link
Contributor

@CoolBots CoolBots left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! I really like your changes, but I also agree with @glen3b, and therefore I request that you implement his suggestions prior to merge.

Grabbing Glen's changes to upstream.
Who uses VIM?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants