Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
384083002:
WIP BSP Tree for 3D Layer Sorting (Closed)
|
Created:
6 years, 5 months ago by troyhildebrandt Modified:
6 years, 4 months ago Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionWIP BSP Tree for 3D Layer Sorting
Still adding test scenarios as I come up with them.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=286499
Patch Set 1 #Patch Set 2 : Small fixes to style/formatting, minor cleanup. #
Total comments: 51
Patch Set 3 : #
Total comments: 18
Patch Set 4 : #
Total comments: 5
Patch Set 5 : #Patch Set 6 : Updated for new DrawPolygon code #Patch Set 7 : Removed heuristic, simplified unittests, new DrawPolygon code #Patch Set 8 : Removed some forgotten debug output #
Total comments: 21
Patch Set 9 : Rebased with DrawPolygon patch #Patch Set 10 : #
Total comments: 12
Patch Set 11 : Added CC_EXPORT to classes, fixed gyp files #
Total comments: 2
Patch Set 12 : #
Messages
Total messages: 26 (0 generated)
Sorry for the delay! Here are a bunch of nits and (likely dumb) questions. https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_controller.cc File cc/output/bsp_controller.cc (right): https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_controller... cc/output/bsp_controller.cc:22: if (node.normal.z() >= 0.f) This is probably a dumb question, but why don't you want (camera - point on plane) DOT (normal) > 0? https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_controller.h File cc/output/bsp_controller.h (right): https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_controller... cc/output/bsp_controller.h:13: enum ViewCompareResult { IN_FRONT, BEHIND }; If we treat the camera plane as a splitting plane, can we use bsp compare result rather than yet another enum? (There are 3, very similar enums in this patch. It would be nice to have 1). https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_controller... cc/output/bsp_controller.h:19: BspController(float compare_thresh, float split_thresh) Could they be static constants in the cc file rather than parameters? https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_controller... cc/output/bsp_controller.h:24: BspCompareResult GetNodePositionRelative(const DrawPolygon& node_a, Perhaps this could be static if the two thresholds weren't params? https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_controller... cc/output/bsp_controller.h:32: scoped_ptr<DrawPolygon>* back) const; Ditto. https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_controller... cc/output/bsp_controller.h:37: // so using it as a splitter instead avoids many unnecessary splits. Is this a common heuristic? https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_tree.cc File cc/output/bsp_tree.cc (right): https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_tree.cc#ne... cc/output/bsp_tree.cc:33: if (list->size() <= 0) Can size < 0? https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_tree.cc#ne... cc/output/bsp_tree.cc:36: float split_weight = 0.0f; nit: max_weight or max_split_weight conveys a bit more meaning. https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_tree.cc#ne... cc/output/bsp_tree.cc:62: float front_weight = 0.0f; Could you choose names for these that involve "max"? As above, I find front_weight and back_weight don't convey a whole lot of meaning. https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_tree.cc#ne... cc/output/bsp_tree.cc:77: if (comparer_result == BSP_FRONT) { switch? https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_tree.cc#ne... cc/output/bsp_tree.cc:90: back_list.push_back(data->take_front()); The front and back behavior is so symmetric that it makes me think that a helper fn taking ptrs to the appropriate stuff could be used instead of duplicating the logic. https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_tree.cc#ne... cc/output/bsp_tree.cc:110: front_list.push_back(new_front.Pass()); These two bits could use that helper, too. https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_tree.cc#ne... cc/output/bsp_tree.cc:124: } else { NOTREACHED(); // or equivalent for the default: case if you switch to switch. } https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_tree.h File cc/output/bsp_tree.h (right): https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_tree.h#new... cc/output/bsp_tree.h:39: void FromList(ScopedPtrVector<DrawPolygon>* list); Would be nice to add comment explaining the difference between these two functions. https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_tree.h#new... cc/output/bsp_tree.h:45: DrawPolygon* item) const { If you define these templatized member functions outside the class, I think you could order public before private in the usual way and improve readability. https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_tree.h#new... cc/output/bsp_tree.h:53: // in this node then walk back then front. Could this be made more concise by storing node ptrs for |first| and |second| nodes and coplanar arrays conditionally upon GetCameraPositionRelative? That would let you use the same traversal code for all of them. https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_tree_unitt... File cc/output/bsp_tree_unittest.cc (right): https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_tree_unitt... cc/output/bsp_tree_unittest.cc:19: enum TestSide { IN_FRONT, BEHIND, COPLANAR }; It would be nice to use the bsp test enum here rather than creating another. (As well as the what-side-am-I-on routine). https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_tree_unitt... cc/output/bsp_tree_unittest.cc:25: virtual float SplitWeight(const DrawPolygon& poly) OVERRIDE { return 0.0f; } I'm not sure I understand why we want to override SplitWeight to return 0.0f here. https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_tree_unitt... cc/output/bsp_tree_unittest.cc:37: std::vector<int>(array, array + sizeof(array) / sizeof(array[0])) Might not need a macro for this? We've got ARRAYSIZE in base. Should just be able to do std::vector<int>(array, array + ARRAYSIZE(array)); https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_tree_unitt... cc/output/bsp_tree_unittest.cc:39: class BspTreeTesting { nit: Present participle is weird for a class name. I'd go with BspTreeTest. https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_tree_unitt... cc/output/bsp_tree_unittest.cc:90: return false; Could we return false before recurring? https://codereview.chromium.org/384083002/diff/20001/cc/quads/draw_polygon.cc File cc/quads/draw_polygon.cc (right): https://codereview.chromium.org/384083002/diff/20001/cc/quads/draw_polygon.cc... cc/quads/draw_polygon.cc:41: area = Area(); Since area is computed in the ctor and is immutable, perhaps Area() should be a free fn in the cc and not visible in the header. https://codereview.chromium.org/384083002/diff/20001/cc/quads/draw_polygon.cc... cc/quads/draw_polygon.cc:78: float z_threshold) { Multiple exit points nested in if/else's in this fn are confusing. I think this would help. if (dot < eps) return getTheNonCoPlanarThingyDoodle(...); if (sign < -z_threshold) return BSP_BACK; if (sign > Z-threshold) return blah; if (...) ... etc. https://codereview.chromium.org/384083002/diff/20001/cc/quads/draw_polygon.cc... cc/quads/draw_polygon.cc:174: } Couldn't this affect area? https://codereview.chromium.org/384083002/diff/20001/cc/quads/draw_polygon.h File cc/quads/draw_polygon.h (right): https://codereview.chromium.org/384083002/diff/20001/cc/quads/draw_polygon.h#... cc/quads/draw_polygon.h:44: bool is_original; // Has this been split yet or is it still intact? Apologies if I just missed it, but where's this used?
https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_controller.cc File cc/output/bsp_controller.cc (right): https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_controller... cc/output/bsp_controller.cc:22: if (node.normal.z() >= 0.f) On 2014/07/16 20:54:32, Ian Vollick wrote: > This is probably a dumb question, but why don't you want (camera - point on > plane) DOT (normal) > 0? You're right, done. https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_controller.h File cc/output/bsp_controller.h (right): https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_controller... cc/output/bsp_controller.h:13: enum ViewCompareResult { IN_FRONT, BEHIND }; On 2014/07/16 20:54:32, Ian Vollick wrote: > If we treat the camera plane as a splitting plane, can we use bsp compare result > rather than yet another enum? (There are 3, very similar enums in this patch. It > would be nice to have 1). Done. https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_controller... cc/output/bsp_controller.h:19: BspController(float compare_thresh, float split_thresh) On 2014/07/16 20:54:32, Ian Vollick wrote: > Could they be static constants in the cc file rather than parameters? Done. https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_controller... cc/output/bsp_controller.h:24: BspCompareResult GetNodePositionRelative(const DrawPolygon& node_a, On 2014/07/16 20:54:32, Ian Vollick wrote: > Perhaps this could be static if the two thresholds weren't params? Done. https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_controller... cc/output/bsp_controller.h:32: scoped_ptr<DrawPolygon>* back) const; On 2014/07/16 20:54:32, Ian Vollick wrote: > Ditto. Done. https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_controller... cc/output/bsp_controller.h:37: // so using it as a splitter instead avoids many unnecessary splits. On 2014/07/16 20:54:32, Ian Vollick wrote: > Is this a common heuristic? I'm not certain if it is, it probably wouldn't hurt to make it a little more intelligent to further improve performance. https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_tree.cc File cc/output/bsp_tree.cc (right): https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_tree.cc#ne... cc/output/bsp_tree.cc:33: if (list->size() <= 0) On 2014/07/16 20:54:32, Ian Vollick wrote: > Can size < 0? Nope, it's unsigned, changed to == 0. https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_tree.cc#ne... cc/output/bsp_tree.cc:36: float split_weight = 0.0f; On 2014/07/16 20:54:33, Ian Vollick wrote: > nit: max_weight or max_split_weight conveys a bit more meaning. Done. https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_tree.cc#ne... cc/output/bsp_tree.cc:62: float front_weight = 0.0f; On 2014/07/16 20:54:32, Ian Vollick wrote: > Could you choose names for these that involve "max"? As above, I find > front_weight and back_weight don't convey a whole lot of meaning. Done. https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_tree.cc#ne... cc/output/bsp_tree.cc:77: if (comparer_result == BSP_FRONT) { On 2014/07/16 20:54:32, Ian Vollick wrote: > switch? Done. https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_tree.cc#ne... cc/output/bsp_tree.cc:90: back_list.push_back(data->take_front()); On 2014/07/16 20:54:33, Ian Vollick wrote: > The front and back behavior is so symmetric that it makes me think that a helper > fn taking ptrs to the appropriate stuff could be used instead of duplicating the > logic. Done. https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_tree.cc#ne... cc/output/bsp_tree.cc:110: front_list.push_back(new_front.Pass()); On 2014/07/16 20:54:32, Ian Vollick wrote: > These two bits could use that helper, too. Done. https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_tree.cc#ne... cc/output/bsp_tree.cc:124: } On 2014/07/16 20:54:33, Ian Vollick wrote: > else { > NOTREACHED(); // or equivalent for the default: case if you switch to switch. > } Done. https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_tree.h File cc/output/bsp_tree.h (right): https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_tree.h#new... cc/output/bsp_tree.h:39: void FromList(ScopedPtrVector<DrawPolygon>* list); On 2014/07/16 20:54:33, Ian Vollick wrote: > Would be nice to add comment explaining the difference between these two > functions. Done. https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_tree.h#new... cc/output/bsp_tree.h:45: DrawPolygon* item) const { On 2014/07/16 20:54:33, Ian Vollick wrote: > If you define these templatized member functions outside the class, I think you > could order public before private in the usual way and improve readability. Simply changed the ordering, no complaints from the compiler. Not sure why I had ordered it that way in the first place. Long story short: done. https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_tree.h#new... cc/output/bsp_tree.h:53: // in this node then walk back then front. On 2014/07/16 20:54:33, Ian Vollick wrote: > Could this be made more concise by storing node ptrs for |first| and |second| > nodes and coplanar arrays conditionally upon GetCameraPositionRelative? That > would let you use the same traversal code for all of them. Done. https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_tree_unitt... File cc/output/bsp_tree_unittest.cc (right): https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_tree_unitt... cc/output/bsp_tree_unittest.cc:19: enum TestSide { IN_FRONT, BEHIND, COPLANAR }; On 2014/07/16 20:54:33, Ian Vollick wrote: > It would be nice to use the bsp test enum here rather than creating another. (As > well as the what-side-am-I-on routine). Done. https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_tree_unitt... cc/output/bsp_tree_unittest.cc:25: virtual float SplitWeight(const DrawPolygon& poly) OVERRIDE { return 0.0f; } On 2014/07/16 20:54:33, Ian Vollick wrote: > I'm not sure I understand why we want to override SplitWeight to return 0.0f > here. Since the weights of polygons affect what order they're placed in the tree, it's important that we don't break unit tests if the heuristic is ever changed. By simply forcing all the weights to be 0, the polygons will all be put into the tree in the same order as they're given, and the unit tests won't be affected by changing heuristics. https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_tree_unitt... cc/output/bsp_tree_unittest.cc:37: std::vector<int>(array, array + sizeof(array) / sizeof(array[0])) On 2014/07/16 20:54:33, Ian Vollick wrote: > Might not need a macro for this? We've got ARRAYSIZE in base. Should just be > able to do std::vector<int>(array, array + ARRAYSIZE(array)); Done. https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_tree_unitt... cc/output/bsp_tree_unittest.cc:39: class BspTreeTesting { On 2014/07/16 20:54:33, Ian Vollick wrote: > nit: Present participle is weird for a class name. I'd go with BspTreeTest. Done. https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_tree_unitt... cc/output/bsp_tree_unittest.cc:90: return false; On 2014/07/16 20:54:33, Ian Vollick wrote: > Could we return false before recurring? Done. https://codereview.chromium.org/384083002/diff/20001/cc/quads/draw_polygon.cc File cc/quads/draw_polygon.cc (right): https://codereview.chromium.org/384083002/diff/20001/cc/quads/draw_polygon.cc... cc/quads/draw_polygon.cc:41: area = Area(); On 2014/07/16 20:54:33, Ian Vollick wrote: > Since area is computed in the ctor and is immutable, perhaps Area() should be a > free fn in the cc and not visible in the header. Done. https://codereview.chromium.org/384083002/diff/20001/cc/quads/draw_polygon.cc... cc/quads/draw_polygon.cc:78: float z_threshold) { On 2014/07/16 20:54:33, Ian Vollick wrote: > Multiple exit points nested in if/else's in this fn are confusing. I think this > would help. > > if (dot < eps) > return getTheNonCoPlanarThingyDoodle(...); > > if (sign < -z_threshold) > return BSP_BACK; > > if (sign > Z-threshold) > return blah; > > if (...) > ... > > etc. > Done. https://codereview.chromium.org/384083002/diff/20001/cc/quads/draw_polygon.cc... cc/quads/draw_polygon.cc:174: } On 2014/07/16 20:54:33, Ian Vollick wrote: > Couldn't this affect area? Yea, I'll take a look and see if I can find a better place/way to calculate area than what we have right now, because you're right, applying a transform can scale/skew/cause clipping to occur and change the area. https://codereview.chromium.org/384083002/diff/20001/cc/quads/draw_polygon.h File cc/quads/draw_polygon.h (right): https://codereview.chromium.org/384083002/diff/20001/cc/quads/draw_polygon.h#... cc/quads/draw_polygon.h:44: bool is_original; // Has this been split yet or is it still intact? On 2014/07/16 20:54:33, Ian Vollick wrote: > Apologies if I just missed it, but where's this used? This is more for the rendering side of things, not seen in this patch. This flag is true for all polygons that haven't undergone a split, and indicates that we can call the original quad drawing code vs. the slightly more expensive custom quad code.
Thanks for these changes! A few more notes. https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_tree_unitt... File cc/output/bsp_tree_unittest.cc (right): https://codereview.chromium.org/384083002/diff/20001/cc/output/bsp_tree_unitt... cc/output/bsp_tree_unittest.cc:25: virtual float SplitWeight(const DrawPolygon& poly) OVERRIDE { return 0.0f; } On 2014/07/18 21:48:26, troyhildebrandt wrote: > On 2014/07/16 20:54:33, Ian Vollick wrote: > > I'm not sure I understand why we want to override SplitWeight to return 0.0f > > here. > > Since the weights of polygons affect what order they're placed in the tree, it's > important that we don't break unit tests if the heuristic is ever changed. By > simply forcing all the weights to be 0, the polygons will all be put into the > tree in the same order as they're given, and the unit tests won't be affected by > changing heuristics. That makes a lot of sense, but it's definitely not obvious. Maybe you could cut and paste this into a comment in the code for future passers-by? https://codereview.chromium.org/384083002/diff/40001/cc/output/bsp_controller.cc File cc/output/bsp_controller.cc (right): https://codereview.chromium.org/384083002/diff/40001/cc/output/bsp_controller... cc/output/bsp_controller.cc:14: static gfx::Point3F camera_point(0.0f, 0.0f, 0.0f); I think that this counts as a static initializer, which is discouraged in chromium code. You should feel completely justified passing in gfx::Point3F() whenever you need the camera's position rather than using this variable. https://codereview.chromium.org/384083002/diff/40001/cc/output/bsp_tree.cc File cc/output/bsp_tree.cc (right): https://codereview.chromium.org/384083002/diff/40001/cc/output/bsp_tree.cc#ne... cc/output/bsp_tree.cc:58: static void BuildHelper(float new_weight, nit: how about UpdateNodeListAndMaxWeight? I know it's a mouthful, but it's a bit more meaningful than BuildHelper. https://codereview.chromium.org/384083002/diff/40001/cc/output/bsp_tree.cc#ne... cc/output/bsp_tree.cc:104: // Front. These comments don't add much; I'd ditch them. https://codereview.chromium.org/384083002/diff/40001/cc/output/bsp_tree_unitt... File cc/output/bsp_tree_unittest.cc (right): https://codereview.chromium.org/384083002/diff/40001/cc/output/bsp_tree_unitt... cc/output/bsp_tree_unittest.cc:17: enum TestSide { IN_FRONT, BEHIND, COPLANAR }; Is this used anymore? https://codereview.chromium.org/384083002/diff/40001/cc/output/bsp_tree_unitt... cc/output/bsp_tree_unittest.cc:48: // tests are complete. Ok to remove this now? https://codereview.chromium.org/384083002/diff/40001/cc/output/bsp_walk_action.h File cc/output/bsp_walk_action.h (right): https://codereview.chromium.org/384083002/diff/40001/cc/output/bsp_walk_actio... cc/output/bsp_walk_action.h:23: // DirectRenderer yet. That will come in a later patch. Please just remove this. You can add it back in the patch in which it's used. https://codereview.chromium.org/384083002/diff/40001/cc/quads/draw_polygon.cc File cc/quads/draw_polygon.cc (right): https://codereview.chromium.org/384083002/diff/40001/cc/quads/draw_polygon.cc... cc/quads/draw_polygon.cc:124: } else { no need for this else, I don't think. https://codereview.chromium.org/384083002/diff/40001/cc/quads/draw_polygon.cc... cc/quads/draw_polygon.cc:135: if (!normal_match || (normal_match && i > 0)) Please be consistent with your braces on one-liners. https://codereview.chromium.org/384083002/diff/40001/cc/quads/draw_polygon.h File cc/quads/draw_polygon.h (right): https://codereview.chromium.org/384083002/diff/40001/cc/quads/draw_polygon.h#... cc/quads/draw_polygon.h:44: bool is_original; // Has this been split yet or is it still intact? This should also be removed until the patch that uses it.
https://codereview.chromium.org/384083002/diff/40001/cc/output/bsp_controller.cc File cc/output/bsp_controller.cc (right): https://codereview.chromium.org/384083002/diff/40001/cc/output/bsp_controller... cc/output/bsp_controller.cc:14: static gfx::Point3F camera_point(0.0f, 0.0f, 0.0f); On 2014/07/19 00:45:01, Ian Vollick wrote: > I think that this counts as a static initializer, which is discouraged in > chromium code. You should feel completely justified passing in gfx::Point3F() > whenever you need the camera's position rather than using this variable. Done. https://codereview.chromium.org/384083002/diff/40001/cc/output/bsp_tree.cc File cc/output/bsp_tree.cc (right): https://codereview.chromium.org/384083002/diff/40001/cc/output/bsp_tree.cc#ne... cc/output/bsp_tree.cc:58: static void BuildHelper(float new_weight, On 2014/07/19 00:45:01, Ian Vollick wrote: > nit: how about UpdateNodeListAndMaxWeight? I know it's a mouthful, but it's a > bit more meaningful than BuildHelper. Done. https://codereview.chromium.org/384083002/diff/40001/cc/output/bsp_tree.cc#ne... cc/output/bsp_tree.cc:104: // Front. On 2014/07/19 00:45:01, Ian Vollick wrote: > These comments don't add much; I'd ditch them. Done. https://codereview.chromium.org/384083002/diff/40001/cc/output/bsp_tree_unitt... File cc/output/bsp_tree_unittest.cc (right): https://codereview.chromium.org/384083002/diff/40001/cc/output/bsp_tree_unitt... cc/output/bsp_tree_unittest.cc:17: enum TestSide { IN_FRONT, BEHIND, COPLANAR }; On 2014/07/19 00:45:01, Ian Vollick wrote: > Is this used anymore? Nope, removed. https://codereview.chromium.org/384083002/diff/40001/cc/output/bsp_tree_unitt... cc/output/bsp_tree_unittest.cc:48: // tests are complete. On 2014/07/19 00:45:01, Ian Vollick wrote: > Ok to remove this now? Removed. https://codereview.chromium.org/384083002/diff/40001/cc/output/bsp_walk_action.h File cc/output/bsp_walk_action.h (right): https://codereview.chromium.org/384083002/diff/40001/cc/output/bsp_walk_actio... cc/output/bsp_walk_action.h:23: // DirectRenderer yet. That will come in a later patch. On 2014/07/19 00:45:01, Ian Vollick wrote: > Please just remove this. You can add it back in the patch in which it's used. Done. https://codereview.chromium.org/384083002/diff/40001/cc/quads/draw_polygon.cc File cc/quads/draw_polygon.cc (right): https://codereview.chromium.org/384083002/diff/40001/cc/quads/draw_polygon.cc... cc/quads/draw_polygon.cc:124: } else { On 2014/07/19 00:45:01, Ian Vollick wrote: > no need for this else, I don't think. Removed, definitely not necessary. https://codereview.chromium.org/384083002/diff/40001/cc/quads/draw_polygon.cc... cc/quads/draw_polygon.cc:135: if (!normal_match || (normal_match && i > 0)) On 2014/07/19 00:45:01, Ian Vollick wrote: > Please be consistent with your braces on one-liners. Done. https://codereview.chromium.org/384083002/diff/40001/cc/quads/draw_polygon.h File cc/quads/draw_polygon.h (right): https://codereview.chromium.org/384083002/diff/40001/cc/quads/draw_polygon.h#... cc/quads/draw_polygon.h:44: bool is_original; // Has this been split yet or is it still intact? On 2014/07/19 00:45:01, Ian Vollick wrote: > This should also be removed until the patch that uses it. Done.
Thanks for these changes. I'm concerned about test coverage for DrawPolygon, but I think this is getting close. https://codereview.chromium.org/384083002/diff/60001/cc/output/bsp_tree.h File cc/output/bsp_tree.h (right): https://codereview.chromium.org/384083002/diff/60001/cc/output/bsp_tree.h#new... cc/output/bsp_tree.h:68: const scoped_ptr<BspNode>& second_child, Passing a const scoped_ptr<BspNode>& rather than a BspNode* here is fine, but it's a bit unusual. Why not pass a raw ptr? https://codereview.chromium.org/384083002/diff/60001/cc/quads/draw_polygon.cc File cc/quads/draw_polygon.cc (right): https://codereview.chromium.org/384083002/diff/60001/cc/quads/draw_polygon.cc... cc/quads/draw_polygon.cc:35: return 0.5f * std::abs(gfx::DotProduct(total, polygon.normal)); The fns in DrawPolygon are probably the most involved, mathmatically and numerically. These really should have unit tests. It would be great to show that these functions are well-conditioned when tough problems are thrown at them. https://codereview.chromium.org/384083002/diff/60001/cc/quads/draw_polygon.h File cc/quads/draw_polygon.h (right): https://codereview.chromium.org/384083002/diff/60001/cc/quads/draw_polygon.h#... cc/quads/draw_polygon.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. 2014. Here and elsewhere.
https://codereview.chromium.org/384083002/diff/60001/cc/output/bsp_tree.h File cc/output/bsp_tree.h (right): https://codereview.chromium.org/384083002/diff/60001/cc/output/bsp_tree.h#new... cc/output/bsp_tree.h:68: const scoped_ptr<BspNode>& second_child, On 2014/07/22 16:12:15, Ian Vollick wrote: > Passing a const scoped_ptr<BspNode>& rather than a BspNode* here is fine, but > it's a bit unusual. Why not pass a raw ptr? Changed. https://codereview.chromium.org/384083002/diff/60001/cc/quads/draw_polygon.cc File cc/quads/draw_polygon.cc (right): https://codereview.chromium.org/384083002/diff/60001/cc/quads/draw_polygon.cc... cc/quads/draw_polygon.cc:35: return 0.5f * std::abs(gfx::DotProduct(total, polygon.normal)); On 2014/07/22 16:12:15, Ian Vollick wrote: > The fns in DrawPolygon are probably the most involved, mathmatically and > numerically. These really should have unit tests. It would be great to show that > these functions are well-conditioned when tough problems are thrown at them. Changed and all part of the DrawPolygon patch separately now.
https://codereview.chromium.org/384083002/diff/140001/cc/output/bsp_controlle... File cc/output/bsp_controller.cc (right): https://codereview.chromium.org/384083002/diff/140001/cc/output/bsp_controlle... cc/output/bsp_controller.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. ಠ_ಠ 2014, here and elsewhere https://codereview.chromium.org/384083002/diff/140001/cc/output/bsp_controller.h File cc/output/bsp_controller.h (right): https://codereview.chromium.org/384083002/diff/140001/cc/output/bsp_controlle... cc/output/bsp_controller.h:13: // Specialization of the BSP Controller abstract class that deals specifically This comment I think should just be removed. https://codereview.chromium.org/384083002/diff/140001/cc/output/bsp_tree.cc File cc/output/bsp_tree.cc (right): https://codereview.chromium.org/384083002/diff/140001/cc/output/bsp_tree.cc#n... cc/output/bsp_tree.cc:26: BspTree::BspTree(BspController* bsp_controller, This is kind of an OO weirdness. BspController isn't actually a polymorphic class. It has no data and only static functions, so it's not like they can be overridden or changed in any way. I think you should probably just remove controller and put those functions in BspTree? Or, if you wanted to keep them separate, have a BspUtil namespace that only has static functions in it and the BspTree can just call the static functions directly rather than via this fake bsp_controller object which doesn't serve any purpose. https://codereview.chromium.org/384083002/diff/140001/cc/output/bsp_tree.cc#n... cc/output/bsp_tree.cc:27: ScopedPtrVector<DrawPolygon>* list) The BspTree takes ownership of this list, right? If so, just make it ScopedPtrVector<DrawPolygon> list so that the caller has to Pass() it in. Also, FromList looks like a noop, so maybe this should be a ScopedPtrDeque to save a copy. https://codereview.chromium.org/384083002/diff/140001/cc/output/bsp_tree.cc#n... cc/output/bsp_tree.cc:57: void BspTree::BuildTree(BspNode* node, ScopedPtrDeque<DrawPolygon>* data) { Can you come up with a better name than "data"? https://codereview.chromium.org/384083002/diff/140001/cc/output/bsp_tree.cc#n... cc/output/bsp_tree.cc:124: Clear(); Why does there need to be an explicit clear? Can this call (and the Clear function) go away? https://codereview.chromium.org/384083002/diff/140001/cc/output/bsp_tree.h File cc/output/bsp_tree.h (right): https://codereview.chromium.org/384083002/diff/140001/cc/output/bsp_tree.h#ne... cc/output/bsp_tree.h:73: for (unsigned int i = 0; i < first_coplanars.size(); i++) { unsigned int => size_t, here and elsewhere https://codereview.chromium.org/384083002/diff/140001/cc/output/bsp_tree_unit... File cc/output/bsp_tree_unittest.cc (right): https://codereview.chromium.org/384083002/diff/140001/cc/output/bsp_tree_unit... cc/output/bsp_tree_unittest.cc:81: for (unsigned int i = 0; i < node->coplanars_back.size(); i++) { unsigned int => size_t https://codereview.chromium.org/384083002/diff/140001/cc/output/bsp_tree_unit... cc/output/bsp_tree_unittest.cc:275: ScopedPtrVector<DrawPolygon> polygon_list; Can you insert these same quads in a different order and verify that they come back in that new order too? https://codereview.chromium.org/384083002/diff/140001/cc/output/bsp_walk_acti... File cc/output/bsp_walk_action.h (right): https://codereview.chromium.org/384083002/diff/140001/cc/output/bsp_walk_acti... cc/output/bsp_walk_action.h:16: template <typename T> Is this templating just historical? It seems like BspTree is always of type DrawPolygon. Can you make this concrete? https://codereview.chromium.org/384083002/diff/140001/cc/output/bsp_walk_acti... cc/output/bsp_walk_action.h:25: virtual void operator()(DrawPolygon* item)OVERRIDE; Please git cl format your patch.
https://codereview.chromium.org/384083002/diff/140001/cc/output/bsp_tree.cc File cc/output/bsp_tree.cc (right): https://codereview.chromium.org/384083002/diff/140001/cc/output/bsp_tree.cc#n... cc/output/bsp_tree.cc:26: BspTree::BspTree(BspController* bsp_controller, On 2014/07/28 21:38:20, enne wrote: > This is kind of an OO weirdness. BspController isn't actually a polymorphic > class. It has no data and only static functions, so it's not like they can be > overridden or changed in any way. > > I think you should probably just remove controller and put those functions in > BspTree? Or, if you wanted to keep them separate, have a BspUtil namespace that > only has static functions in it and the BspTree can just call the static > functions directly rather than via this fake bsp_controller object which doesn't > serve any purpose. Removed the BspController, it's an artifact of an older design. https://codereview.chromium.org/384083002/diff/140001/cc/output/bsp_tree.cc#n... cc/output/bsp_tree.cc:27: ScopedPtrVector<DrawPolygon>* list) On 2014/07/28 21:38:20, enne wrote: > The BspTree takes ownership of this list, right? If so, just make it > ScopedPtrVector<DrawPolygon> list so that the caller has to Pass() it in. Also, > FromList looks like a noop, so maybe this should be a ScopedPtrDeque to save a > copy. Now BspTree constructor takes a ScopedPtrDeque* instead, removed FromList. https://codereview.chromium.org/384083002/diff/140001/cc/output/bsp_tree.cc#n... cc/output/bsp_tree.cc:27: ScopedPtrVector<DrawPolygon>* list) On 2014/07/28 21:38:20, enne wrote: > The BspTree takes ownership of this list, right? If so, just make it > ScopedPtrVector<DrawPolygon> list so that the caller has to Pass() it in. Also, > FromList looks like a noop, so maybe this should be a ScopedPtrDeque to save a > copy. Removed FromList, made the constructor do minimal work by taking a ScopedPtrDeque*. https://codereview.chromium.org/384083002/diff/140001/cc/output/bsp_tree.cc#n... cc/output/bsp_tree.cc:57: void BspTree::BuildTree(BspNode* node, ScopedPtrDeque<DrawPolygon>* data) { On 2014/07/28 21:38:20, enne wrote: > Can you come up with a better name than "data"? Done. https://codereview.chromium.org/384083002/diff/140001/cc/output/bsp_tree.cc#n... cc/output/bsp_tree.cc:124: Clear(); On 2014/07/28 21:38:19, enne wrote: > Why does there need to be an explicit clear? Can this call (and the Clear > function) go away? Removed. https://codereview.chromium.org/384083002/diff/140001/cc/output/bsp_tree.h File cc/output/bsp_tree.h (right): https://codereview.chromium.org/384083002/diff/140001/cc/output/bsp_tree.h#ne... cc/output/bsp_tree.h:73: for (unsigned int i = 0; i < first_coplanars.size(); i++) { On 2014/07/28 21:38:20, enne wrote: > unsigned int => size_t, here and elsewhere Done. https://codereview.chromium.org/384083002/diff/140001/cc/output/bsp_tree_unit... File cc/output/bsp_tree_unittest.cc (right): https://codereview.chromium.org/384083002/diff/140001/cc/output/bsp_tree_unit... cc/output/bsp_tree_unittest.cc:81: for (unsigned int i = 0; i < node->coplanars_back.size(); i++) { On 2014/07/28 21:38:20, enne wrote: > unsigned int => size_t Done. https://codereview.chromium.org/384083002/diff/140001/cc/output/bsp_tree_unit... cc/output/bsp_tree_unittest.cc:275: ScopedPtrVector<DrawPolygon> polygon_list; On 2014/07/28 21:38:20, enne wrote: > Can you insert these same quads in a different order and verify that they come > back in that new order too? Done. They don't come in the new order, however, because if the order_index of one polygon is less than that of the other, it comes in front of that one in the sorting process, so we get back the exact same 0,1,2 ordering despite the different input order. https://codereview.chromium.org/384083002/diff/140001/cc/output/bsp_walk_acti... File cc/output/bsp_walk_action.h (right): https://codereview.chromium.org/384083002/diff/140001/cc/output/bsp_walk_acti... cc/output/bsp_walk_action.h:16: template <typename T> On 2014/07/28 21:38:20, enne wrote: > Is this templating just historical? It seems like BspTree is always of type > DrawPolygon. Can you make this concrete? Done. https://codereview.chromium.org/384083002/diff/140001/cc/output/bsp_walk_acti... cc/output/bsp_walk_action.h:25: virtual void operator()(DrawPolygon* item)OVERRIDE; On 2014/07/28 21:38:20, enne wrote: > Please git cl format your patch. Done.
https://codereview.chromium.org/384083002/diff/180001/cc/cc.gyp File cc/cc.gyp (right): https://codereview.chromium.org/384083002/diff/180001/cc/cc.gyp#newcode247 cc/cc.gyp:247: 'output/bsp_compare_result.h', Isn't this supposed to be in the DrawPolygon patch? https://codereview.chromium.org/384083002/diff/180001/cc/cc_tests.gyp File cc/cc_tests.gyp (left): https://codereview.chromium.org/384083002/diff/180001/cc/cc_tests.gyp#oldcode71 cc/cc_tests.gyp:71: 'quads/draw_polygon_unittest.cc', I think you shouldn't remove this? https://codereview.chromium.org/384083002/diff/180001/cc/cc_tests.gyp File cc/cc_tests.gyp (right): https://codereview.chromium.org/384083002/diff/180001/cc/cc_tests.gyp#newcode61 cc/cc_tests.gyp:61: 'output/bsp_tree.cc', These files shouldn't be added other than the new unit test? https://codereview.chromium.org/384083002/diff/180001/cc/output/bsp_tree.cc File cc/output/bsp_tree.cc (right): https://codereview.chromium.org/384083002/diff/180001/cc/output/bsp_tree.cc#n... cc/output/bsp_tree.cc:19: node_data = data.Pass(); nit: use an initializer for this, i.e. BspNode::BspNode(scoped_ptr<DrawPolygon> data) : node_data(data.Pass()) { } https://codereview.chromium.org/384083002/diff/180001/cc/output/bsp_tree.cc#n... cc/output/bsp_tree.cc:58: bool split_result = false; Can you scope this closer to where it's used? I know it's a switch statement, but you could just use {} around the code after the case so you don't need it declared so far away here. https://codereview.chromium.org/384083002/diff/180001/cc/output/bsp_walk_acti... File cc/output/bsp_walk_action.h (right): https://codereview.chromium.org/384083002/diff/180001/cc/output/bsp_walk_acti... cc/output/bsp_walk_action.h:24: virtual void operator()(DrawPolygon* item)OVERRIDE; This is formatted wrong. I'm a little confused why clang-format isn't doing the right thing here, but this seems like a bug and I'll go file it when I get a chance. Can you add a space before OVERRIDE?
https://codereview.chromium.org/384083002/diff/180001/cc/cc.gyp File cc/cc.gyp (right): https://codereview.chromium.org/384083002/diff/180001/cc/cc.gyp#newcode247 cc/cc.gyp:247: 'output/bsp_compare_result.h', On 2014/07/29 00:28:08, enne wrote: > Isn't this supposed to be in the DrawPolygon patch? Yea, this should be included in the DrawPolygon patch, I removed it. https://codereview.chromium.org/384083002/diff/180001/cc/cc_tests.gyp File cc/cc_tests.gyp (left): https://codereview.chromium.org/384083002/diff/180001/cc/cc_tests.gyp#oldcode71 cc/cc_tests.gyp:71: 'quads/draw_polygon_unittest.cc', On 2014/07/29 00:28:08, enne wrote: > I think you shouldn't remove this? Added back. https://codereview.chromium.org/384083002/diff/180001/cc/cc_tests.gyp File cc/cc_tests.gyp (right): https://codereview.chromium.org/384083002/diff/180001/cc/cc_tests.gyp#newcode61 cc/cc_tests.gyp:61: 'output/bsp_tree.cc', On 2014/07/29 00:28:08, enne wrote: > These files shouldn't be added other than the new unit test? Removed. https://codereview.chromium.org/384083002/diff/180001/cc/output/bsp_tree.cc File cc/output/bsp_tree.cc (right): https://codereview.chromium.org/384083002/diff/180001/cc/output/bsp_tree.cc#n... cc/output/bsp_tree.cc:19: node_data = data.Pass(); On 2014/07/29 00:28:08, enne wrote: > nit: use an initializer for this, i.e. > > BspNode::BspNode(scoped_ptr<DrawPolygon> data) > : node_data(data.Pass()) { > } Done. https://codereview.chromium.org/384083002/diff/180001/cc/output/bsp_tree.cc#n... cc/output/bsp_tree.cc:58: bool split_result = false; On 2014/07/29 00:28:08, enne wrote: > Can you scope this closer to where it's used? I know it's a switch statement, > but you could just use {} around the code after the case so you don't need it > declared so far away here. Done. https://codereview.chromium.org/384083002/diff/180001/cc/output/bsp_walk_acti... File cc/output/bsp_walk_action.h (right): https://codereview.chromium.org/384083002/diff/180001/cc/output/bsp_walk_acti... cc/output/bsp_walk_action.h:24: virtual void operator()(DrawPolygon* item)OVERRIDE; On 2014/07/29 00:28:08, enne wrote: > This is formatted wrong. I'm a little confused why clang-format isn't doing the > right thing here, but this seems like a bug and I'll go file it when I get a > chance. Can you add a space before OVERRIDE? Done.
lgtm https://codereview.chromium.org/384083002/diff/240001/cc/output/bsp_walk_acti... File cc/output/bsp_walk_action.h (right): https://codereview.chromium.org/384083002/diff/240001/cc/output/bsp_walk_acti... cc/output/bsp_walk_action.h:21: class CC_EXPORT BspWalkActionToVector : public BspWalkAction { Can you add a comment saying that the BspTree owns the DrawPolygons that get returned in its list and so its lifetime must be preserved while the vector of DrawPolygons is being used?
https://codereview.chromium.org/384083002/diff/240001/cc/output/bsp_walk_acti... File cc/output/bsp_walk_action.h (right): https://codereview.chromium.org/384083002/diff/240001/cc/output/bsp_walk_acti... cc/output/bsp_walk_action.h:21: class CC_EXPORT BspWalkActionToVector : public BspWalkAction { On 2014/07/29 21:35:41, enne wrote: > Can you add a comment saying that the BspTree owns the DrawPolygons that get > returned in its list and so its lifetime must be preserved while the vector of > DrawPolygons is being used? Done.
The CQ bit was checked by thildebr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thildebr@chromium.org/384083002/280001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by enne@chromium.org
The CQ bit was unchecked by enne@chromium.org
The CQ bit was checked by danakj@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thildebr@chromium.org/384083002/280001
Message was sent while issue was closed.
Change committed as 286499
Message was sent while issue was closed.
Oops, I forgot to ask you to change this description so it didn't say WIP anymore and was more informative. Ah, well.
Message was sent while issue was closed.
On 2014/07/30 15:32:09, enne wrote: > Oops, I forgot to ask you to change this description so it didn't say WIP > anymore and was more informative. Ah, well. Oh ya, I'll remember that for the perf tests for sure. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
