full bounds behavior has changed in Piccolo 1.3

[mro]

Originally reported on Google Code with ID 161

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?

Reported by cmalley@pixelzoom.com on 2010-02-09 02:37:15

---

• Attachment: DebugFullBounds.java

[mro]

Piccolo has an interesting definition of empty. It's possible to have nodes that have

girth and position but still be considered empty.

Reported by allain.lalonde on 2010-02-09 03:37:33

[mro]

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.

Reported by reids%colorado.edu@gtempaccount.com on 2010-02-09 03:44:11

[mro]

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.

Reported by reids%colorado.edu@gtempaccount.com on 2010-02-09 05:50:41

[mro]

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.

Reported by allain.lalonde on 2010-02-09 14:57:36

[mro]

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.

Reported by reids%colorado.edu@gtempaccount.com on 2010-02-09 15:06:49

[mro]

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.

Reported by allain.lalonde on 2010-02-09 15:18:10

[mro]

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.

Reported by cmalley@pixelzoom.com on 2010-02-09 16:42:11

[mro]

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

Reported by heuermh on 2010-02-09 16:44:33

[mro]

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.

Reported by cmalley@pixelzoom.com on 2010-02-09 16:45:11

[mro]

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.

Reported by allain.lalonde on 2010-02-09 16:45:36

[mro]

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 ;-)

Reported by cmalley@pixelzoom.com on 2010-02-09 16:48:28

[mro]

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.

Reported by heuermh on 2010-02-09 16:59:57

[mro]

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.

Reported by reids%colorado.edu@gtempaccount.com on 2010-02-09 17:06:37

[mro]

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

Marking this ticket as verified.

Reported by cmalley@pixelzoom.com on 2010-02-10 19:54:39

• Status changed: Verified