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

Feature/cleanup #63

Closed
wants to merge 110 commits into from
Closed

Feature/cleanup #63

wants to merge 110 commits into from

Conversation

Fox32
Copy link
Contributor

@Fox32 Fox32 commented Mar 16, 2014

I started this pull request with the goal to remove call chaining (#59) but I also took a look at "some" other things:

  • Remove call chaining (breaking change, but the Dart Editor gives very good guidance here!)
  • Rename some parts to match the Dart Style guide (constants, constructors, ...) (I only deprecated them)
  • Removed some methods that didn't provide a real benefit (e.g. Sphere.copyOriginDirection)
  • Formatting
  • Analyser warnings
  • Remove duplicate code (e.g. the plus operator of a vector can share the same code as VectorN.add)
  • Add more documentation comments (especially for the collision detection objects)
  • Improve code in a way that dart2js can generate better code. I took these suggestions as a start. Here is a example of dart2js code that changed, the old code:
dot$1: [function(other) {
      var t1, t2, t3, t4, t5, t6, t7;
      t1 = this.storage;
      t2 = t1[0];
      t3 = other.get$storage();
      t4 = t3.length;
      if (0 >= t4)
        return H.ioore(t3, 0);
      t5 = t3[0];
      t6 = t1[1];
      if (1 >= t4)
        return H.ioore(t3, 1);
      t7 = t3[1];
      t1 = t1[2];
      if (2 >= t4)
        return H.ioore(t3, 2);
      return t2 * t5 + t6 * t7 + t1 * t3[2];
    }, "call$1", "get$dot", 2, 0, null, 109, []],

and the new code:

    dot$1: [function(other) {
      var otherStorage, t1;
      otherStorage = other.get$_storage();
      t1 = this._storage;
      return t1[0] * otherStorage[0] + t1[1] * otherStorage[1] + t1[2] * otherStorage[2];
    }, "call$1", "get$dot", 2, 0, null, 109, []],

In this case range checks could be removed, in other cases less calls to getters are made. It also has a small influence on code size, but I don't think that these are significant. For the future it would be nice to have a test bench to compare the performance of changes in both the DartVM and Dart2Js. Not sure what to choose as a benchmark here, maybe some sort of simulation?

Fox32 added 28 commits March 16, 2014 11:53
- More documentation
- Formatting
- Remove call chaining
- More documentation
- Formatting
- Remove call chaining
- Tune the code to allow dart2js to generate better output (also for Aabb2)
- More documentation
- Formatting
- Remove call chaining
- Tune the code to allow dart2js to generate better output
- Remove duplicate code => Aabb3.getPN
- Change documentation comment style
- Formatting
- Tune the code to allow dart2js to generate better output
- Change type annotations from num to double (and the toDouble calls) to match the style of the rest of the library
- Don't use the * operator on matrix as it requires type checking
- More documentation
- Formatting
- Fix normalConstant constructor caseing
- More documentation
- Formatting
- Remove copyOriginDirection
- Tune the code to allow dart2js to generate better output
- More documentation
- Formatting
- Tune the code to allow dart2js to generate better output
- Fix bug in copyFrom
- More documentation
- Formatting
- Remove  copyOriginDirection method (yes it was really named that way...)
- Formatting
- Tune the code to allow dart2js to generate better output
- Remove call chaining
- Remove duplicated code from operators
- Formatting
- Tune the code to allow dart2js to generate better output
- Remove call chaining
- Remove duplicated code from operators
- Formatting
- Tune the code to allow dart2js to generate better output
- Remove call chaining
- Remove duplicated code from operators
- Formatting
- Tune the code to allow dart2js to generate better output
- Remove call chaining
- Remove duplicated code
…into feature/cleanup

Conflicts:
	lib/src/vector_math/opengl.dart
Keep up with my changes
- Formatting
- Tune the code to allow dart2js to generate better output
- Remove call chaining
- Remove duplicated code from operators
- Formatting
- Tune the code to allow dart2js to generate better output
- Remove call chaining
- Remove duplicated code from operators
double get t => storage[1];
double get p => storage[2];
Vector4 get xxxx => new Vector4(storage[0], storage[0], storage[0], storage[0]
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's with this weird style?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used the new formatter that has some issues left. I can fix them by hand or leave them in until the formatter is fixed

dhenneke and others added 7 commits August 26, 2014 16:06
Add Quad intersection to Obb3 and Aabb3
Add tests for Obb3 intersects Triangle
Fix Quad intersection
Fix Quad.copyNormal test, the used quad was not valid
Add Quad.copyTriangles test
@Fox32
Copy link
Contributor Author

Fox32 commented Aug 28, 2014

Thanke @dhenneke for helping me with additional Aabb3 and Obb3 intersecion tests!

@johnmccutchan
Copy link
Collaborator

Hi,

This is too big to review. Please break it into small pieces.

@Fox32
Copy link
Contributor Author

Fox32 commented Aug 28, 2014

I already plan that for some time, but haven't find the right way yet as some changes are very big on their own. But I will manage that somehow...

@johnmccutchan
Copy link
Collaborator

I recommend separating breaking changes from non-breaking changes and then further splitting up those change sets.

@Fox32
Copy link
Contributor Author

Fox32 commented Apr 6, 2015

My plan is to wait until the bigger cleanup PRs by others are finished. Currently merge conflicts are too annoying, especially the formatting.Then my plan is to do get vector_math back on a similar level as my fork. But I would prefer to make feature centered PRs, here is my plan:

@kevmoo
Copy link
Collaborator

kevmoo commented Apr 6, 2015

I like it, @Fox32.

Keep an eye on the new Box2d library as you move forward - https://github.com/google/box2d.dart

@kevmoo
Copy link
Collaborator

kevmoo commented Apr 6, 2015

@Fox32 Do you have plans to track performance changes for these tweaks?

@Fox32
Copy link
Contributor Author

Fox32 commented Apr 6, 2015

@kevmoo I think I try to do the performance things later, but that Box2d would be a nice benchmark for some of the core classes, sadly Box2d isn't covering much of vector_math.
Beside that I would try to reduce the amount of type and bounds checks of the generade javascript code.

Do you have any bigger changes to vector_math in the pipeline?

@kevmoo
Copy link
Collaborator

kevmoo commented Apr 6, 2015

@Fox32 Nope.

@kevmoo
Copy link
Collaborator

kevmoo commented Apr 6, 2015

@Fox32 Keep things formatted using dartfmt with each of your commits so we can keep style-related churn to a minimum

@Fox32
Copy link
Contributor Author

Fox32 commented Apr 7, 2015

@kevmoo @johnmccutchan
I updated the list above with my current progress. I think we have enough PRs for now and I will wait till all are review, corrected, and merged. I starting to have dependencies between the PRs that makes further work to complicated for now.

@Fox32
Copy link
Contributor Author

Fox32 commented Apr 13, 2015

Everything, except call chaining, is now included \o/
I will close this one.

@Fox32 Fox32 closed this Apr 13, 2015
@johnmccutchan
Copy link
Collaborator

@Fox32 Thank you very much for your great work. Looking forward to 2.0 without call chaining.

@Fox32 Fox32 deleted the feature/cleanup branch February 2, 2016 21:19
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.

None yet

4 participants