Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2551263002:
Don't add duplicate points when clipping (Closed)
|
Created:
4 years ago by Peter Mayo Modified:
3 years, 11 months ago CC:
cc-bugs_chromium.org, chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't add duplicate points when clipping
Neighbours can clip to the same infinity. Using two of the same
vertex in a polygon can cause the computed normal to be (0,0,0),
which causes badness and failures.
BUG=626095
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2551263002
Cr-Commit-Position: refs/heads/master@{#443296}
Committed: https://chromium.googlesource.com/chromium/src/+/9dfe043a1722d857b7e92e4d646d974c4f76f5cf
Patch Set 1 #Patch Set 2 : Rebased #Patch Set 3 : First test #
Total comments: 7
Patch Set 4 : Add approximate truncation, with tests. #
Total comments: 9
Patch Set 5 : rebase #Patch Set 6 : Readability and review changes #
Total comments: 14
Patch Set 7 : rebase #Patch Set 8 : flackr comments #10 #
Total comments: 11
Patch Set 9 : flackr comments #12 #Patch Set 10 : danakj comments #13 #Patch Set 11 : danakj comments #15 #
Total comments: 2
Patch Set 12 : danakj comments #15-ii #Patch Set 13 : danakj comments #15-iii #Patch Set 14 : Move Test function into class for export #
Dependent Patchsets: Messages
Total messages: 39 (12 generated)
Patchset #2 (id:20001) has been deleted
petermayo@chromium.org changed reviewers: + danakj@chromium.org, flackr@chromium.org
Another piece of fallout from debugging.
Looks good, can you add a test of such a case?
Added a first test. https://codereview.chromium.org/2551263002/diff/60001/cc/base/math_util.cc File cc/base/math_util.cc (right): https://codereview.chromium.org/2551263002/diff/60001/cc/base/math_util.cc#ne... cc/base/math_util.cc:162: if (clipped_quad[*num_vertices_in_clipped_quad - 1] == new_vertex) These are floating point values, and should use an approximately equal to, rather than absolute equality.
https://codereview.chromium.org/2551263002/diff/60001/cc/base/math_util.cc File cc/base/math_util.cc (right): https://codereview.chromium.org/2551263002/diff/60001/cc/base/math_util.cc#ne... cc/base/math_util.cc:162: if (clipped_quad[*num_vertices_in_clipped_quad - 1] == new_vertex) On 2016/12/08 at 19:37:39, Peter Mayo wrote: > These are floating point values, and should use an approximately equal to, rather than absolute equality. If the problem case is clipping to the same infinity do they not end up exactly equal? https://codereview.chromium.org/2551263002/diff/60001/cc/base/math_util_unitt... File cc/base/math_util_unittest.cc (right): https://codereview.chromium.org/2551263002/diff/60001/cc/base/math_util_unitt... cc/base/math_util_unittest.cc:465: gfx::QuadF src_quad(gfx::PointF(0.0f, 100.0f), gfx::PointF(0.0f, -100.0f), It might be worth explicitly pointing out the two vertices which clip to the same point in a comment. It's the last two right? https://codereview.chromium.org/2551263002/diff/60001/cc/base/math_util_unitt... cc/base/math_util_unittest.cc:481: printf("]\n"); debugging code?
https://codereview.chromium.org/2551263002/diff/60001/cc/base/math_util.cc File cc/base/math_util.cc (right): https://codereview.chromium.org/2551263002/diff/60001/cc/base/math_util.cc#ne... cc/base/math_util.cc:162: if (clipped_quad[*num_vertices_in_clipped_quad - 1] == new_vertex) On 2016/12/08 20:24:01, flackr wrote: > On 2016/12/08 at 19:37:39, Peter Mayo wrote: > > These are floating point values, and should use an approximately equal to, > rather than absolute equality. > > If the problem case is clipping to the same infinity do they not end up exactly > equal? Inifinity is a single value in the non-parametric code, but becomes a vaguer region in the parametric case - in order to stay planar in the 3d case. https://codereview.chromium.org/2551263002/diff/60001/cc/base/math_util_unitt... File cc/base/math_util_unittest.cc (right): https://codereview.chromium.org/2551263002/diff/60001/cc/base/math_util_unitt... cc/base/math_util_unittest.cc:465: gfx::QuadF src_quad(gfx::PointF(0.0f, 100.0f), gfx::PointF(0.0f, -100.0f), On 2016/12/08 20:24:01, flackr wrote: > It might be worth explicitly pointing out the two vertices which clip to the > same point in a comment. It's the last two right? Actually a comment about the whole scenario may be even better. https://codereview.chromium.org/2551263002/diff/60001/cc/base/math_util_unitt... cc/base/math_util_unittest.cc:481: printf("]\n"); On 2016/12/08 20:24:01, flackr wrote: > debugging code? Yup, hence 'First ... Definitely more to come, and needed to discuss the other point.
Just a few comments and naming suggestions. https://codereview.chromium.org/2551263002/diff/80001/cc/base/math_util.cc File cc/base/math_util.cc (right): https://codereview.chromium.org/2551263002/diff/80001/cc/base/math_util.cc#ne... cc/base/math_util.cc:158: static inline bool approx(const float f, const float g) { Maybe approximatelyEqual to be verbose? https://codereview.chromium.org/2551263002/diff/80001/cc/base/math_util.cc#ne... cc/base/math_util.cc:159: static const float epsilon_scale = 0.00001; nit: 0.00001f https://codereview.chromium.org/2551263002/diff/80001/cc/base/math_util.cc#ne... cc/base/math_util.cc:160: return std::abs(f - g) < nit: I think <= might be safer in case of precision errors. https://codereview.chromium.org/2551263002/diff/80001/cc/base/math_util_unitt... File cc/base/math_util_unittest.cc (right): https://codereview.chromium.org/2551263002/diff/80001/cc/base/math_util_unitt... cc/base/math_util_unittest.cc:461: #define approx(x, y) IsApproximatelyForUnitTesting(x, y) nit: Maybe this should be something like EXPECT_APPROX_EQ and EXPECT_APPROX_NE (kind of an amusing concept, approximately not equal :-) ) which could map to the appropriate EXPECT_TRUE / EXPECT_FALSE construction.
PTAL, when you can. https://codereview.chromium.org/2551263002/diff/80001/cc/base/math_util.cc File cc/base/math_util.cc (right): https://codereview.chromium.org/2551263002/diff/80001/cc/base/math_util.cc#ne... cc/base/math_util.cc:158: static inline bool approx(const float f, const float g) { On 2016/12/08 23:31:06, flackr (OOO) wrote: > Maybe approximatelyEqual to be verbose? I think nearlyTheSame covers all of the overloaded functions more consistently. One of the advantages of the abbreviation was that it fit many readings of the conditions for the different types. https://codereview.chromium.org/2551263002/diff/80001/cc/base/math_util.cc#ne... cc/base/math_util.cc:159: static const float epsilon_scale = 0.00001; On 2016/12/08 23:31:06, flackr (OOO) wrote: > nit: 0.00001f Acknowledged. https://codereview.chromium.org/2551263002/diff/80001/cc/base/math_util.cc#ne... cc/base/math_util.cc:160: return std::abs(f - g) < On 2016/12/08 23:31:06, flackr (OOO) wrote: > nit: I think <= might be safer in case of precision errors. I worry that it would worsely cover over inappropriately close tolerances. It seems that if you are sensitive to the last possible mantissa value you have missed the point of using the function. Having < without the = highlighted the problems around zero earlier, rather than later. I think it would be better to dodge that sort of pitfall earlier in the future too. https://codereview.chromium.org/2551263002/diff/80001/cc/base/math_util_unitt... File cc/base/math_util_unittest.cc (right): https://codereview.chromium.org/2551263002/diff/80001/cc/base/math_util_unitt... cc/base/math_util_unittest.cc:461: #define approx(x, y) IsApproximatelyForUnitTesting(x, y) On 2016/12/08 23:31:06, flackr (OOO) wrote: > nit: Maybe this should be something like EXPECT_APPROX_EQ and EXPECT_APPROX_NE > (kind of an amusing concept, approximately not equal :-) ) which could map to > the appropriate EXPECT_TRUE / EXPECT_FALSE construction. Picked EXPECT_SIMILAR... and EXPECT_DISSIMILAR... https://codereview.chromium.org/2551263002/diff/80001/cc/base/math_util_unitt... cc/base/math_util_unittest.cc:482: EXPECT_TRUE(approx(gfx::PointF(0.0f, 0.0f), gfx::PointF(0.0f, 0.0f))); We can use a cover to get rid of the constructor noise here and in the 3d case.
https://codereview.chromium.org/2551263002/diff/120001/cc/base/math_util.cc File cc/base/math_util.cc (right): https://codereview.chromium.org/2551263002/diff/120001/cc/base/math_util.cc#n... cc/base/math_util.cc:159: static const float epsilon_scale = 0.00001f; nit: Can you add a short comment as to why / how we vary what near means with respect to epsilon_scale? https://codereview.chromium.org/2551263002/diff/120001/cc/base/math_util.cc#n... cc/base/math_util.cc:183: if (isNearlyTheSame(clipped_quad[0], new_vertex)) Instead of comparing every added vertex with vertex 0 could we add a check in MapClippedQuad and MapClippedQuad3d after all of the vertices have been added that the last is equal to the first and if so reduce the number of vertices to clip the last one? https://codereview.chromium.org/2551263002/diff/120001/cc/base/math_util.cc#n... cc/base/math_util.cc:322: void MathUtil::MapClippedQuad(const gfx::Transform& transform, It's interesting that there seem to be no calls to the 2d version of MapClippedQuad. Seems like we should maybe remove it. https://codereview.chromium.org/2551263002/diff/120001/cc/base/math_util.cc#n... cc/base/math_util.cc:442: return (*num_vertices_in_clipped_quad >= 4); It seems like noone uses this return value and we exclude polygons with fewer than 3 points in direct_renderer.cc. Can we change this to void? https://codereview.chromium.org/2551263002/diff/120001/cc/base/math_util_unit... File cc/base/math_util_unittest.cc (right): https://codereview.chromium.org/2551263002/diff/120001/cc/base/math_util_unit... cc/base/math_util_unittest.cc:562: // the eypoint and checks to make sure we build a triangle. We used to build nit: eye point https://codereview.chromium.org/2551263002/diff/120001/cc/base/math_util_unit... cc/base/math_util_unittest.cc:586: // a degnerate quad. The quirk here is that the two shared points are first nit: degenerate https://codereview.chromium.org/2551263002/diff/120001/cc/base/math_util_unit... cc/base/math_util_unittest.cc:607: // behind us. We don't want two verticies at inifinity crossing in and out nit: infinity
https://codereview.chromium.org/2551263002/diff/120001/cc/base/math_util.cc File cc/base/math_util.cc (right): https://codereview.chromium.org/2551263002/diff/120001/cc/base/math_util.cc#n... cc/base/math_util.cc:159: static const float epsilon_scale = 0.00001f; On 2017/01/11 18:23:57, flackr wrote: > nit: Can you add a short comment as to why / how we vary what near means with > respect to epsilon_scale? Done. https://codereview.chromium.org/2551263002/diff/120001/cc/base/math_util.cc#n... cc/base/math_util.cc:183: if (isNearlyTheSame(clipped_quad[0], new_vertex)) On 2017/01/11 18:23:57, flackr wrote: > Instead of comparing every added vertex with vertex 0 could we add a check in > MapClippedQuad and MapClippedQuad3d after all of the vertices have been added > that the last is equal to the first and if so reduce the number of vertices to > clip the last one? This expands the scope in which the "isNearlyTheSame" is used, but cuts down on execution, so yes I think this is a net good idea. https://codereview.chromium.org/2551263002/diff/120001/cc/base/math_util.cc#n... cc/base/math_util.cc:322: void MathUtil::MapClippedQuad(const gfx::Transform& transform, On 2017/01/11 18:23:57, flackr wrote: > It's interesting that there seem to be no calls to the 2d version of > MapClippedQuad. Seems like we should maybe remove it. A fine idea for another CL. I have a few more for this file. https://codereview.chromium.org/2551263002/diff/120001/cc/base/math_util.cc#n... cc/base/math_util.cc:442: return (*num_vertices_in_clipped_quad >= 4); On 2017/01/11 18:23:57, flackr wrote: > It seems like noone uses this return value and we exclude polygons with fewer > than 3 points in direct_renderer.cc. Can we change this to void? A fine idea for another CL. I have a few more for this file. https://codereview.chromium.org/2551263002/diff/120001/cc/base/math_util_unit... File cc/base/math_util_unittest.cc (right): https://codereview.chromium.org/2551263002/diff/120001/cc/base/math_util_unit... cc/base/math_util_unittest.cc:562: // the eypoint and checks to make sure we build a triangle. We used to build On 2017/01/11 18:23:57, flackr wrote: > nit: eye point Done. Here and elsewhere. https://codereview.chromium.org/2551263002/diff/120001/cc/base/math_util_unit... cc/base/math_util_unittest.cc:586: // a degnerate quad. The quirk here is that the two shared points are first On 2017/01/11 18:23:57, flackr wrote: > nit: degenerate Done. Here and elsewhere https://codereview.chromium.org/2551263002/diff/120001/cc/base/math_util_unit... cc/base/math_util_unittest.cc:607: // behind us. We don't want two verticies at inifinity crossing in and out On 2017/01/11 18:23:57, flackr wrote: > nit: infinity Done.
lgtm https://codereview.chromium.org/2551263002/diff/160001/cc/base/math_util.cc File cc/base/math_util.cc (right): https://codereview.chromium.org/2551263002/diff/160001/cc/base/math_util.cc#n... cc/base/math_util.cc:162: // for the base of the scale too. spelling nit: s/tow/two s/ase/as s/asolute/absolute https://codereview.chromium.org/2551263002/diff/160001/cc/base/math_util.cc#n... cc/base/math_util.cc:381: if (isNearlyTheSame(clipped_quad[0], I think canonical style is to use && rather than nest dependent conditions.
https://codereview.chromium.org/2551263002/diff/160001/cc/base/math_util.cc File cc/base/math_util.cc (right): https://codereview.chromium.org/2551263002/diff/160001/cc/base/math_util.cc#n... cc/base/math_util.cc:158: static inline bool isNearlyTheSame(const float f, const float g) { fix name style https://codereview.chromium.org/2551263002/diff/160001/cc/base/math_util.cc#n... cc/base/math_util.cc:381: if (isNearlyTheSame(clipped_quad[0], On 2017/01/11 22:48:28, flackr wrote: > I think canonical style is to use && rather than nest dependent conditions. yes, this throughout. https://codereview.chromium.org/2551263002/diff/160001/cc/base/math_util.cc#n... cc/base/math_util.cc:986: bool IsNearlyTheSameForUnitTesting(const float left, const float right) { ForTesting is the suffix that presubmits look for. Why are these only for testing, would they be bad to use for other cases? There's a bunch of epsilon float comparison stuff sprinkled around the place should we use this more widely?
https://codereview.chromium.org/2551263002/diff/160001/cc/base/math_util.cc File cc/base/math_util.cc (right): https://codereview.chromium.org/2551263002/diff/160001/cc/base/math_util.cc#n... cc/base/math_util.cc:158: static inline bool isNearlyTheSame(const float f, const float g) { On 2017/01/11 23:12:06, danakj (slow) wrote: > fix name style Just the capital, or do you have a specific suggestion on a change? https://codereview.chromium.org/2551263002/diff/160001/cc/base/math_util.cc#n... cc/base/math_util.cc:162: // for the base of the scale too. On 2017/01/11 22:48:28, flackr wrote: > spelling nit: s/tow/two s/ase/as s/asolute/absolute enoD https://codereview.chromium.org/2551263002/diff/160001/cc/base/math_util.cc#n... cc/base/math_util.cc:381: if (isNearlyTheSame(clipped_quad[0], On 2017/01/11 23:12:06, danakj (slow) wrote: > On 2017/01/11 22:48:28, flackr wrote: > > I think canonical style is to use && rather than nest dependent conditions. > > yes, this throughout. Throughout the CL, done. https://codereview.chromium.org/2551263002/diff/160001/cc/base/math_util.cc#n... cc/base/math_util.cc:986: bool IsNearlyTheSameForUnitTesting(const float left, const float right) { On 2017/01/11 23:12:06, danakj (slow) wrote: > ForTesting is the suffix that presubmits look for. Why are these only for > testing, would they be bad to use for other cases? Yes, because it would stress the definitions. Didn't know about the presubmits looking for the name - are they doing something for them we want done here? What I see immediately is them giving an incorrect warning: " ** Presubmit Warnings ** " You might be calling functions intended only for testing from " production code. It is OK to ignore this warning if you know what " you are doing, as the heuristics used to detect the situation are " not perfect. The commit queue will not block on this warning. " cc/base/math_util.cc:984 " bool IsNearlyTheSameForTesting(const gfx::PointF& left, \ " cc/base/math_util.cc:989 " bool IsNearlyTheSameForTesting(const gfx::Point3F& left, > > There's a bunch of epsilon float comparison stuff sprinkled around the place > should we use this more widely? Near enough is a context specific concept, so sharing the values is not necessarily a long-term benefit.
https://codereview.chromium.org/2551263002/diff/160001/cc/base/math_util.cc File cc/base/math_util.cc (right): https://codereview.chromium.org/2551263002/diff/160001/cc/base/math_util.cc#n... cc/base/math_util.cc:158: static inline bool isNearlyTheSame(const float f, const float g) { On 2017/01/11 23:47:04, Peter Mayo wrote: > On 2017/01/11 23:12:06, danakj (slow) wrote: > > fix name style > > Just the capital, or do you have a specific suggestion on a change? just the capital. but also consting a float parameter is a bit unusual now that i read this again.
https://codereview.chromium.org/2551263002/diff/160001/cc/base/math_util.cc File cc/base/math_util.cc (right): https://codereview.chromium.org/2551263002/diff/160001/cc/base/math_util.cc#n... cc/base/math_util.cc:986: bool IsNearlyTheSameForUnitTesting(const float left, const float right) { On 2017/01/11 23:47:04, Peter Mayo wrote: > On 2017/01/11 23:12:06, danakj (slow) wrote: > > ForTesting is the suffix that presubmits look for. Why are these only for > > testing, would they be bad to use for other cases? > > Yes, because it would stress the definitions. Didn't know about the presubmits > looking for the name - are they doing something for them we want done here? > > What I see immediately is them giving an incorrect warning: > " ** Presubmit Warnings ** > " You might be calling functions intended only for testing from > " production code. It is OK to ignore this warning if you know what > " you are doing, as the heuristics used to detect the situation are > " not perfect. The commit queue will not block on this warning. > " cc/base/math_util.cc:984 > " bool IsNearlyTheSameForTesting(const gfx::PointF& left, \ > " cc/base/math_util.cc:989 > " bool IsNearlyTheSameForTesting(const gfx::Point3F& left, > > > > > There's a bunch of epsilon float comparison stuff sprinkled around the place > > should we use this more widely? > > Near enough is a context specific concept, so sharing the values is not > necessarily a long-term benefit. They look for non-test code calling the function and create an error when it happens. So you should use that syntax here instead of ForUnitTesting. Are you saying your patch generates errors when these are named ForTesting?
On 2017/01/11 23:53:36, danakj (slow) wrote: > https://codereview.chromium.org/2551263002/diff/160001/cc/base/math_util.cc > File cc/base/math_util.cc (right): > > https://codereview.chromium.org/2551263002/diff/160001/cc/base/math_util.cc#n... > cc/base/math_util.cc:986: bool IsNearlyTheSameForUnitTesting(const float left, > const float right) { > On 2017/01/11 23:47:04, Peter Mayo wrote: > > On 2017/01/11 23:12:06, danakj (slow) wrote: > > > ForTesting is the suffix that presubmits look for. Why are these only for > > > testing, would they be bad to use for other cases? > > > > Yes, because it would stress the definitions. Didn't know about the > presubmits > > looking for the name - are they doing something for them we want done here? > > > > What I see immediately is them giving an incorrect warning: > > " ** Presubmit Warnings ** > > " You might be calling functions intended only for testing from > > " production code. It is OK to ignore this warning if you know what > > " you are doing, as the heuristics used to detect the situation are > > " not perfect. The commit queue will not block on this warning. > > " cc/base/math_util.cc:984 > > " bool IsNearlyTheSameForTesting(const gfx::PointF& left, \ > > " cc/base/math_util.cc:989 > > " bool IsNearlyTheSameForTesting(const gfx::Point3F& left, > > > > > > > > There's a bunch of epsilon float comparison stuff sprinkled around the place > > > should we use this more widely? > > > > Near enough is a context specific concept, so sharing the values is not > > necessarily a long-term benefit. > > They look for non-test code calling the function and create an error when it > happens. So you should use that syntax here instead of ForUnitTesting. Are you > saying your patch generates errors when these are named ForTesting? Yes - it improperly detects the multiline signature definition as an invocation. Easy enough to go around anyway.
On 2017/01/11 23:51:54, danakj (slow) wrote: > https://codereview.chromium.org/2551263002/diff/160001/cc/base/math_util.cc > File cc/base/math_util.cc (right): > > https://codereview.chromium.org/2551263002/diff/160001/cc/base/math_util.cc#n... > cc/base/math_util.cc:158: static inline bool isNearlyTheSame(const float f, > const float g) { > On 2017/01/11 23:47:04, Peter Mayo wrote: > > On 2017/01/11 23:12:06, danakj (slow) wrote: > > > fix name style > > > > Just the capital, or do you have a specific suggestion on a change? > > just the capital. > > but also consting a float parameter is a bit unusual now that i read this again. Yes - here wasn't where I lost that style argument. Here it isn't so bad since it mimics the overloaded ones that take a const ref to more complex types. ref to a const float would have been more similar and more weird at the same time. I was of half a mind to change it on the last edit.
On Wed, Jan 11, 2017 at 6:56 PM, <petermayo@chromium.org> wrote: > On 2017/01/11 23:53:36, danakj (slow) wrote: > > https://codereview.chromium.org/2551263002/diff/160001/cc/ > base/math_util.cc > > File cc/base/math_util.cc (right): > > > > > https://codereview.chromium.org/2551263002/diff/160001/cc/ > base/math_util.cc#newcode986 > > cc/base/math_util.cc:986: bool IsNearlyTheSameForUnitTesting(const > float left, > > const float right) { > > On 2017/01/11 23:47:04, Peter Mayo wrote: > > > On 2017/01/11 23:12:06, danakj (slow) wrote: > > > > ForTesting is the suffix that presubmits look for. Why are these > only for > > > > testing, would they be bad to use for other cases? > > > > > > Yes, because it would stress the definitions. Didn't know about the > > presubmits > > > looking for the name - are they doing something for them we want done > here? > > > > > > What I see immediately is them giving an incorrect warning: > > > " ** Presubmit Warnings ** > > > " You might be calling functions intended only for testing from > > > " production code. It is OK to ignore this warning if you know what > > > " you are doing, as the heuristics used to detect the situation are > > > " not perfect. The commit queue will not block on this warning. > > > " cc/base/math_util.cc:984 > > > " bool IsNearlyTheSameForTesting(const gfx::PointF& left, \ > > > " cc/base/math_util.cc:989 > > > " bool IsNearlyTheSameForTesting(const gfx::Point3F& left, > > > > > > > > > > > There's a bunch of epsilon float comparison stuff sprinkled around > the > place > > > > should we use this more widely? > > > > > > Near enough is a context specific concept, so sharing the values is not > > > necessarily a long-term benefit. > > > > They look for non-test code calling the function and create an error > when it > > happens. So you should use that syntax here instead of ForUnitTesting. > Are you > > saying your patch generates errors when these are named ForTesting? > > Yes - it improperly detects the multiline signature definition as an > invocation. > > Easy enough to go around anyway. > Oh no that's sad. Well don't check in presubmit errors, others changing the file will get them. Can you shorten names to fit on a line? > > > https://codereview.chromium.org/2551263002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Wed, Jan 11, 2017 at 6:59 PM, <petermayo@chromium.org> wrote: > On 2017/01/11 23:51:54, danakj (slow) wrote: > > https://codereview.chromium.org/2551263002/diff/160001/cc/ > base/math_util.cc > > File cc/base/math_util.cc (right): > > > > > https://codereview.chromium.org/2551263002/diff/160001/cc/ > base/math_util.cc#newcode158 > > cc/base/math_util.cc:158: static inline bool isNearlyTheSame(const float > f, > > const float g) { > > On 2017/01/11 23:47:04, Peter Mayo wrote: > > > On 2017/01/11 23:12:06, danakj (slow) wrote: > > > > fix name style > > > > > > Just the capital, or do you have a specific suggestion on a change? > > > > just the capital. > > > > but also consting a float parameter is a bit unusual now that i read this > again. > > Yes - here wasn't where I lost that style argument. Here it isn't so bad > since > it mimics the overloaded ones that take a const ref to more complex types. > ref > to a const float would have been more similar and more weird at the same > time. > Right we don't pass primitive types by ref, they always go by value :) > > I was of half a mind to change it on the last edit. > > https://codereview.chromium.org/2551263002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2551263002/diff/220001/cc/base/math_util.h File cc/base/math_util.h (right): https://codereview.chromium.org/2551263002/diff/220001/cc/base/math_util.h#ne... cc/base/math_util.h:338: bool IsNearlyTheSameForTesting(const float left, const float right); These need to lose const to match. Another patchset coming when the compile/test/upload cycle is complete.
LGTM with the const floats being just floats https://codereview.chromium.org/2551263002/diff/220001/cc/base/math_util.h File cc/base/math_util.h (right): https://codereview.chromium.org/2551263002/diff/220001/cc/base/math_util.h#ne... cc/base/math_util.h:338: bool IsNearlyTheSameForTesting(const float left, const float right); On 2017/01/12 00:05:13, Peter Mayo wrote: > These need to lose const to match. > > Another patchset coming when the compile/test/upload cycle is complete. Yep yep
The CQ bit was checked by petermayo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from flackr@chromium.org Link to the patchset: https://codereview.chromium.org/2551263002/#ps240001 (title: "danakj comments #15-ii")
The CQ bit was unchecked by petermayo@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by petermayo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from flackr@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2551263002/#ps260001 (title: "danakj comments #15-iii")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/01/12 01:44:22, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) PTAL I'm not sure if it's best to move these into the class or to decorate them with XXX_EXPORT (Was surprised this wasn't a local problem but appeared on the linux bot - guess I have component build turned off.)
Oh I didn't notice they are not in the class, they should be, so this LGTM
The CQ bit was checked by petermayo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from flackr@chromium.org Link to the patchset: https://codereview.chromium.org/2551263002/#ps280001 (title: "Move Test function into class for export")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 280001, "attempt_start_ts": 1484241882848800,
"parent_rev": "4791ebef19fb91bccca86fc9755e5cb2d72e2cda", "commit_rev":
"9dfe043a1722d857b7e92e4d646d974c4f76f5cf"}
Message was sent while issue was closed.
Description was changed from ========== Don't add duplicate points when clipping Neighbours can clip to the same infinity. Using two of the same vertex in a polygon can cause the computed normal to be (0,0,0), which causes badness and failures. BUG=626095 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Don't add duplicate points when clipping Neighbours can clip to the same infinity. Using two of the same vertex in a polygon can cause the computed normal to be (0,0,0), which causes badness and failures. BUG=626095 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2551263002 Cr-Commit-Position: refs/heads/master@{#443296} Committed: https://chromium.googlesource.com/chromium/src/+/9dfe043a1722d857b7e92e4d646d... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:280001) as https://chromium.googlesource.com/chromium/src/+/9dfe043a1722d857b7e92e4d646d... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
