full bounds behavior has changed in Piccolo 1.3

[GoogleCodeExporter]

What steps will reproduce the problem?

Run the attached example program (DebugFullBounds) under Piccolo 1.3-rc1 or
1.3-rc2, and compare with behavior prior to r923 (which fixed issue 155). 
The javadoc in the example explains the details of the issue.

What is the expected output? What do you see instead?

I would expect the full bounds computation to be unchanged pre- and
post-r923.  Instead, a parent node's bounds always affect the full bounds
computation, even when those bounds are empty.  Prior to r923, empty bounds
had no affect on the full bounds computation.

Please use labels and text to provide additional information.

This seems like a significant change in behavior, which could affect layout
code.  We've already found one PhET application that was affected.

Questions:
o Was this behavior change intentional/anticipated when issue 155 was fixed?  
o Or was pre-r963 behavior correct? (I'm not entirely sure what's correct.)
o Is this a reason to fail 1.3-rc2?

Original issue reported on code.google.com by cmal...@pixelzoom.com on 9 Feb 2010 at 2:37

Attachments:

• DebugFullBounds.java

[GoogleCodeExporter]

Piccolo has an interesting definition of empty. It's possible to have nodes 
that have 
girth and position but still be considered empty.

Original comment by allain.lalonde on 9 Feb 2010 at 3:37

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

I've been using the DebugFullBounds.java attached above in order to reason 
about the
desired behavior, and found another unexpected but related behavior.  When I 
set the
bounds.x=10, bounds.y=30, bounds.w=52, bounds.h=0, the red rectangle is 
visible, but
reports its bounds as PBounds[EMPTY].  I've been implicitly assuming that any 
node
with an empty bounds should (a) not be visible and (b) not be incorporated into
bounds computations.  However, based on the above behavior in the 
DebugFullBounds, it
seems more like a PPath(Line2D.Double(0,0,100,0) or 
PPath(Rectangle2D(0,0,100,0))
should indeed be visible, have a non-empty bounds and be incorporated into 
bounds
computations.  A case that I'm also concerned about is the case in which there 
is a
PNode with children nodes, but no direct content (other than the children); in 
that
case, it seems like the node itself shouldn't be visible and shouldn't enter 
into
bounds computations.  I'm not sure how to handle that case symmetrically with 
the
one-dimensional PPath cases above, and I'm not sure what solution will best 
resolve
both this issue 161 and  issue 155.

Original comment by reids%co...@gtempaccount.com on 9 Feb 2010 at 3:44

[GoogleCodeExporter]

Here's a proposed behavior that I think is predominantly consistent with the 
pre-r963
behavior, and that makes more sense to me.

1. Container PNodes (i.e. nodes that have children but do not define any content
themselves) should have PBounds[EMPTY] and not be taken into consideration when
computing FullBounds.
2. Content PNodes (i.e. PPath, PText or other nodes that define their own 
content and
may additionally have children) should never have PBounds[EMPTY] and should 
always be
taken into consideration when computing FullBounds.  This means that
PPath(Rectangle(0,0,0,0)) would have PBounds(0,0,0,0) and would impact layout, 
even
if it is not displayed on the screen under any zoom parameters.  Similarly for
PText(""), an empty PImage etc.

This seems like it would provide reasonable and reliable behavior for performing
layouts.  This would also make the graphical output in DebugFullBounds.java 
correct,
and its readout of PBounds[EMPTY] for 0-width 0-height rectangles incorrect.  
This
behavior is also consistent with the desired behavior reported in Issue 155.

If we agree that this is the best semantics for layout, then we will next need 
to
discuss the best way of implementing this behavior.

Original comment by reids%co...@gtempaccount.com on 9 Feb 2010 at 5:50

[GoogleCodeExporter]

We've tried as much as possible to cause no breaking changes while fixing bugs, 
but... the fix to Issue 155 has obviously done so.

With hindsight, implementing any change to the way PNode computes its full 
bounds is 
an error in a point release since no matter what there's a change it'll break 
client 
code.

Unless there are objections, I'm going to rollback the change for Issue 155, 
reopen 
it and mark it for inclusion in 2.0 since it's a breaking change.

Original comment by allain.lalonde on 9 Feb 2010 at 2:57

[GoogleCodeExporter]

I agree with the recommendation in Comment 5 to roll back the fix for issue 155 
for
the 1.3 release.  Should we plan on a 1.3rc3 for this?  Also, I think we should
discuss the proposed semantics in Comment 4 for inclusion in 2.0; let me know 
if that
warrants a separate ticket, or should be subsumed by issue 155 or other tickets.

Original comment by reids%co...@gtempaccount.com on 9 Feb 2010 at 3:06

[GoogleCodeExporter]

Regarding comment 4... I'd be OK with that approach, but I think that it ought 
to be 
the exception rather than the rule. Children classes who wish to abstain from 
full 
bounds computations should override the appropriate methods. Maybe even 
composite only.

Original comment by allain.lalonde on 9 Feb 2010 at 3:18

[GoogleCodeExporter]

I also agree with Allain's recommendation to roll back the fix for issue 155, 
and
reopen that ticket.  Keeping the behavior the same in a point release is the 
first
priority, no matter what we think about what might be "better" behavior.

Changing the behavior is more appropriate to address in 2.0.  So regarding Sam's
proposal in comment 4... I would need to think about it some more, and I think 
the
idea may need some refinement.  So yes, a new ticket to track that as a feature
proposal and it's possible inclusion in 2.0.

Finally... Do we need to fail 1.3-rc2 because of this?  Technically, I think 
so. 
I've already voted to support the release, but Sam has not voted.

Original comment by cmal...@pixelzoom.com on 9 Feb 2010 at 4:42

[GoogleCodeExporter]

Any of us can change our votes on 1.3-rc2 if necessary.

Original comment by heue...@gmail.com on 9 Feb 2010 at 4:44

[GoogleCodeExporter]

Btw... Just for the record, we've found a second PhET application broken by this
change.  And there are probably others that we haven't noticed yet, since the
breakage is sometimes subtle.

Original comment by cmal...@pixelzoom.com on 9 Feb 2010 at 4:45

[GoogleCodeExporter]

I've reverted and re-opened.

Regarding the rc, technically yes we should re-vote.

Since this is the only change, if things run off the trunk, it might just mean 
more 
work for Michael.

Original comment by allain.lalonde on 9 Feb 2010 at 4:45

[GoogleCodeExporter]

Thanks for the clarification on the rc votes, heuermh.  I assumed that "binding"
meant a commitment that couldn't be changed.  Nice to know that this commit 
includes
revert ;-)

Original comment by cmal...@pixelzoom.com on 9 Feb 2010 at 4:48

[GoogleCodeExporter]

I don't mind putting out new RCs, it's better late than never.

Binding votes are those from project committers.

Non-binding votes are those from non-committers.  A -1 non-binding vote does not
necessarily block a release, although it would be inconsiderate to do so.

Original comment by heue...@gmail.com on 9 Feb 2010 at 4:59

[GoogleCodeExporter]

I've opened new issue 162 to address discussion and implementation of improved
semantics for full bounds and PBounds(EMPTY) for Milestone 2.0.  Therefore, 
since
Allain has already reverted the problem (see comment 11), this ticket seems 
resolved.

Original comment by reids%co...@gtempaccount.com on 9 Feb 2010 at 5:06

[GoogleCodeExporter]

Verified that reverting r923 fixes the example application (DebugFullBounds) 
and our
production applications.

Marking this ticket as verified.

Original comment by cmal...@pixelzoom.com on 10 Feb 2010 at 7:54

• Changed state: Verified