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

Proposed update to ofRectangle::scaleTo - now with more scaling modes. #1513

Merged
merged 26 commits into from Oct 1, 2012

Conversation

bakercp
Copy link
Member

@bakercp bakercp commented Aug 27, 2012

Since implementing @ofZach's suggestion for the recently added ofRectangle::scaleTo (formerly known as ofRectangle::scaleIntoMe) method (see PR #1332), I have started using it quite extensively.

In using it, I have found a lot of other situations where other scaling modes based on an existing rectangle would be quite useful. While there are other ways to do it, this would be useful when scaling an image into an FBO like this:

void Layer::drawFrameIntoFbo(ofImage* image, ofFbo* fbo, ofRectScaleMode mode) {
    fbo->begin();
    ofClear(0,0,0,0);

    ofRectangle fboRect(0,0,fbo->getWidth(),fbo->getHeight());
    ofRectangle imageRect(0,0,image->getWidth(),image->getHeight());

    ofRectangle scaledRect = fboRect.scaleToMe(imageRect,mode);

    image->draw(scaledRect);

    fbo->end(); // end the fbo
}

This PR proposes the addition of an enum called ofRectScaleMode. And a change from ofRectangle::ScaleIntoMe() to ofRectangle::ScaleToMe() to capture the idea that ofRectangle can be scaled by other ofRectangles in a variety of ways. It would be nice to get this method name change in before the next release so we won't have to worry about backward compatibility.

The original ofRectangle::ScaleIntoMe() might best be described as a "fit" scaling operation. The new ofRectangle::ScaleToMe() uses OF_RECTSCALEMODE_FIT by default.

Below is the new enum with comments. Comments are also included in the commit, and should possibly be removed before any merges.

enum ofRectScaleMode{
    // fits the SUBJECT rect INSIDE the TARGET rect.
    // Preserves SUBJECTS's aspect ratio.
    // Final Subject's Area <= Target's Area.
    // Subject's Center == Target's Center
    OF_RECTSCALEMODE_FIT     = 0,
    // FILLS the TARGET rect with the SUBJECT rect.
    // Preserves the SUBJECT's aspect ratio.
    // Subject's Area >= Target's Area.
    // Subject's Center == Target's Center
    OF_RECTSCALEMODE_FILL    = 1,
    // Preserves the SUBJECT's aspect ratio.
    // Subject's Area is Unchanged
    // Subject's Center == Target's Center
    OF_RECTSCALEMODE_CENTER  = 2, // centers the subject
    // Can CHANGE the SUBJECT's aspect ratio.
    // Subject's Area == Target's Area
    // Subject's Center == Target's Center
    OF_RECTSCALEMODE_STRETCH = 3, // simply matches the target dims
};

If you would like to see it in action and try it out, here is a test app https://gist.github.com/3492711.

If you want to compare it to a another familiar paradigm, on a mac go to System Preferences -> Desktop & Screensaver -> Desktop and you can scale your desktop image similarly. I believe there is a similar option in windows + Linux, but I don't have it in front of me.

Now ofRectangle::ScaleToMe.  
More scaling options via ofRectScaleMode.
@bilderbuchi
Copy link
Member

On Ubuntu 12.04 there's Tile (tile smaller images, for larger images this == CENTER), Zoom (your FILL), Centre (your CENTER), Scale (your FIT), Fill (your STRETCH), Span (spanning for dual monitors, otherwise == Scale).
Is your nomenclature identical in behaviour to MacOS's Desktop scaling options?

@julapy
Copy link
Member

julapy commented Aug 28, 2012

this feature is very useful indeed... something im always using in my projects.
https://github.com/julapy/ofxJulapyUtils/blob/master/ofxResizeUtil.h

ive always used the term CROP instead of FILL but FILL also makes perfect sense.

@bakercp
Copy link
Member Author

bakercp commented Aug 28, 2012

@bilderbuchi I left out tile because it's a little more specialized and I wasn't sure how to have a method signature of ofRectangle that could encompass all of them and include tiling. If you think tile is important and have a suggestion on how it might be included and the syntax of scaleInMe -- maybe it will give you a vector full of ofRectangles?

The names are Mac-biased, but I'm not -- so if there is a more intuitive naming scheme (I kind of feel like the Linux names are actually more intuitive), I'd prefer that.

@julapy I think I'd prefer Linux's Linux Scale/Mac Fit over Crop -- as crop seems to imply that the aspect ratio of the original rect might be lost. Your ofxResizeUtil class is really nice. While the math seems to be the same, I do prefer how you explicitly named wRatio / hRatio before calculating the MAX/MIN. I think it would be easier to read than mine.

@ofTheo
Copy link
Member

ofTheo commented Aug 28, 2012

this looks great.
if @ofZach is okay with the name change - then this looks good to merge.

@bilderbuchi
Copy link
Member

This was no request to include tiling (nor spanning), I just wanted to be complete in the list of options available on Ubuntu. :-)

Nomenclature-wise, CENTER and STRETCH are intuitive, but I think for distinguishing the two AR-preserving fitting modes, I find neither fill/fit nor zoom/scale very intuitive. I think maybe it's a better idea to got with something like FIT_INSIDE/FIT_OUTSIDE, like the source rectangle can be thought of as being inside (as you already say in your comment) or outside the target rectangle.

@kylemcdonald
Copy link
Contributor

looks good to me too. definitely going to switch to using this in the future. it would be nice to have a demo that just shows different rectangles drawn in different colors using the different modes.

regarding nomenclature, i've previously called "FIT" "fit inside" and "FILL" "fit outside".

it's not clear to me what the difference is between "OF_RECTSCALEMODE_STRETCH" and just saying rectA = rectB

also i wanted to mention: some of your initial post says ScaleIntoMe but all your code uses the (correct) lowercase initial s.

@bakercp
Copy link
Member Author

bakercp commented Aug 28, 2012

@kylemcdonald I'm happy to fill out the demo code here https://gist.github.com/3492711 into full-fledged example / demo.

Any more votes for the FIT_INSIDE / FIT_OUTSIDE? @kylemcdonald and @bilderbuchi to be in favor. Others haven't registered a strong opinion. @ofZach @ofTheo @julapy thoughts?

@kylemcdonald -- There is no difference "OF_RECTSCALEMODE_STRETCH" and just saying rectA = rectB. It is just there for enum completeness. For instance, in my use case, I have a toggle that switches between all enum values when I'm scaling an image into an fbo. Without the STRETCH option, it would require extra code and another special case to do that operation.

Sorry about the capital S. All typos. The code is king here.

@ofTheo
Copy link
Member

ofTheo commented Aug 28, 2012

I prefer FIT / FILL - but that is because it is similar to what its called in InDesign.
http://tinytutorials.files.wordpress.com/2011/11/screen-shot-2011-11-11-at-10-22-45-am.png

I'm easy though :)

@bakercp
Copy link
Member Author

bakercp commented Aug 28, 2012

I guess I prefer FIT / FILL but would ideally like the name to self-describe the preservation of the subject's aspect ratio. I don't want people to have to read the comments every time. Adding "XXX_PROPORTIONALLY" or "XXX_PRESERVE_ASPECT" gets a bit long though. Maybe scaling via fit/fill/center already implies aspect ratio preservation and stretching doesn't?

@ofTheo
Copy link
Member

ofTheo commented Aug 28, 2012

I think FIT, FILL and STRETCH_TO_FILL imply just the last is non-aspect ratio respecting.

@bakercp
Copy link
Member Author

bakercp commented Aug 28, 2012

✓+ for the STRETCH_TO_FILL solution.

@bilderbuchi
Copy link
Member

for me, "scale" implies AR-preservation, and "stretch" implies non-AR preservation. "fit/fill" is more ambiguous (which is probably why, in Theo's InDesign screenshot, it's called "fit proportionally"). So maybe, inmproving my initial proposal, SCALE_INSIDE/OUTSIDE?

edit: FIT/FILL/STRETCH_TO_FILL also sounds ok I guess.

@bakercp
Copy link
Member Author

bakercp commented Aug 30, 2012

Hi all, just a note -- I've continued working with this, paying careful attention to the emerging alignment enum needs within the ofxFont (where much of this has been coming from) and ofxGui (widget alignment, label alignment, etc) contexts and I'm starting to believe that we need a little bit more flexibility in the way to describe aspect ratio mode, alignment and scaling. The point is -- I'd recommend not pulling this just yet. I'm planning to post some updates here shortly.

@bakercp
Copy link
Member Author

bakercp commented Aug 31, 2012

Still working on a few things. Don't pull :)

…are based on Qt and could, if desired, be combined into a single bit-wise combined alignment flag.
- x/y are now references to an underlying ofPoint
  for easier ofPoint/ofVec manipulations of an ofRectangl.
  (addresses issue openframeworks#821 by @danomatika)
- Added setters for x/y/width/height/position (vec)
- Changed the variable names on translateXXX to dx/dy (delta x/y)
- Added translateX, translateY
- Added scaleWidth / scaleHeight for independent control.
- changed scaleToMe to simply be scaleTo.
  + remove personal pronoun.
  + Scales itself, rather than returning the scaled version of another ofRectangle.
- Added scaleTo functions that are more generic, using the new ofAspectRatioMode.
  + This allows more flexible scaling with alternate alignments.
  + Overridden functions provide easier access w/o changing defaults.
- Added alignTo functions that now work with the new ofAlignHorz and ofAlignVert enums.
  + contain functions for aligning to other rects, points, and a single value,
    based on a the anchor.
- Moved the overridden operators to the bottom of the implementation file / header.
- Added getLeft()/getRight()/getTop()/getBottom().
  + since our ofRectangle class supports negative width/height, these offer
    more intuitive interaction with known edges.
- Added getHorzAnchor() / getVertAnchor() to get edges by using the new alignment enums.  Throws errors if the alignment enums are invalid.
- getPosition and getPositionRef added.
- Moved getCenter() to live with the other getters.
@bakercp
Copy link
Member Author

bakercp commented Sep 5, 2012

Hello all, I just pushed some significant changes to this pull request based on our discussions. In addition to a few new getters and setters and a response to issue #821, I also added a suite of alignment tools and more complex scaling tools. Please read through the full commit messages for specific comments. Also, please check out the branch and run the example program. It should be self explanatory (and pretty cool if you like lining things up and scaling things.).

One note, I searched long and hard to figure out the best way to do alignment in oF. My goal was to mimic the simple alignment constants in Processing (i.e. align left works for text and other things as well), but after much work, I am pretty convinced that text alignment needs its own enum -- particularly with all of the fancy options for justification. In the future, methods that need an ofTextAlignHorz/ofTextAlignVert could be overridden to also receive ofAlignVert/ofAlignHorz.

My motivation for these alignment functions is ultimately based on my development of ofxFont but I believe it will come in handy for developing Guis and tasks.

@@ -363,11 +363,52 @@ enum ofWindowMode{
OF_GAME_MODE = 2
};

enum ofAspectRatioMode {
Copy link
Member Author

Choose a reason for hiding this comment

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

These names were borrowed from Qt's similar enum.

@@ -10,22 +11,26 @@
ofRectangle::~ ofRectangle(){}

//----------------------------------------------------------
ofRectangle::ofRectangle(float px, float py, float w, float h){
ofRectangle::ofRectangle(float px, float py, float w, float h) : x(position.x), y(position.y)
{
set(px,py,w,h);
Copy link
Member

Choose a reason for hiding this comment

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

not a big thing but curious why the brackets were dropped down a line?

Copy link
Member Author

Choose a reason for hiding this comment

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

No idea. I'll fix them.

@ofTheo
Copy link
Member

ofTheo commented Sep 26, 2012

mostly this looks good to me, apart from some small naming questions.
one thing that was discussed at the code review was canonicalize - can we find a better name for it? :)

@bakercp
Copy link
Member Author

bakercp commented Sep 26, 2012

Yes, I know canonicalize is a bit awkward and not very self-descriptive. I used it because that's how what the process is called in other frameworks and geometry examples I've found on the net. I'll do a bit more research on this and make some proposals.

@ofTheo
Copy link
Member

ofTheo commented Sep 26, 2012

thanks - that all sounds great!

@bilderbuchi
Copy link
Member

just a btw, the uncrustifier is ready to go, just waiting for someone to pull the trigger. (a helper script for it is in #1542).

@bakercp
Copy link
Member Author

bakercp commented Sep 26, 2012

So, I've now changed my vote to normalize() / isNormalized() const / getNormalized() const to replace the canonicalize() / isCanonicalized() const / getCanonicalized() const.

Canonicalize seems to be a "classic" way to do it (i.e. from early C days). Normalize has much better representation across platforms and frameworks. Initially I did not choose normalize b/c I didn't want it to be confused with vector normalization, but, I think it will be clear enough for the people that need to use it.

Anyway, not a lot of reason to read below, but they are there for your enjoyment.

Canonicalize has some precedents (e.g.)

http://libcinder.org/docs/v0.8.4/classcinder_1_1_rect_t.html
http://www.csl.sri.com/users/gilham/clim/page32.html
http://rss.acs.unt.edu/Rdoc/library/MLEcens/html/canon2real.html

But Normalize has a lot of precedents too (e.g.):

http://msdn.microsoft.com/en-us/library/61b19s95(v=vs.80).aspx
http://www.ranorex.com/Documentation/Ranorex/html/M_Ranorex_Core_Geometry_Normalize.htm
http://25yearsofprogramming.com/borland40msdos/doublerect.htm
http://qgis.sourceforge.net/qgis_api/html/classQgsRect.html#a15
http://help.eclipse.org/helios/topic/org.eclipse.platform.doc.isv/reference/api/org/eclipse/jface/util/Geometry.html#normalize(org.eclipse.swt.graphics.Rectangle)
http://harmattan-dev.nokia.com/docs/library/html/qt4/qrect.html#normalized
http://pepper.troll.no/s60prereleases/doc/qrect-qt3.html#normalize

@bilderbuchi
Copy link
Member

Indeed, at least to me "normalize" sounds like it would return a square or something like that.
Is there precedent about "standardize()"? Is there a fitting verb/formulation equivalent to "consistent"?

@damian0815
Copy link
Contributor

IIRC one of the issues we had in Maine with canonicalize is that no-one could figure out without looking at the source what it was for. i don't think renaming it to normalize solves this.

the problem is that what it is doing (enforcing non-negative width/height) only makes sense if you make the (non-obvious) assumption that width and height may be negative numbers.

my vote would be for a verbose function name like makeWidthAndHeightNonNegative.

however i think a stronger case can be made for removing the function completely, as the existence of such a function would seem to imply that ofRectangle is actually capable of correctly dealing with rectangles with negative width/height (i would not at all assume that it is).

@ofTheo
Copy link
Member

ofTheo commented Sep 26, 2012

Normalize makes more sense, but agree on the potential for confusion.

what about: makeDimensionsPositive() - isDimensionsPositive() ?
agree with damian in that clarity over elegance :)

I can see the need for non-positive dimensions, @bakercp does ofRectangle fully support that at the moment?

@bakercp
Copy link
Member Author

bakercp commented Sep 26, 2012

One argument for the normalized nomenclature is that it is used quite extensively in other frameworks. Another is that most new coders (assumption alert!) won't need/use the function -- and coders that do need it are likely more experienced and coming (again assuming) from other platforms that used the same lingo -- or they will take a look at the documentation or source and see what it does.

In terms of support for non-positive dimensions -- it is currently completely supported (with careful attention paid to that fact). You'll see lots of fabs and min/max checks that keep track of the logical right/left/top/bottom (i.e. it can't be assumed that ((x + width) == right), etc that wouldn't be required if we forced width >= 0 && height >= 0. It is also supported in drawing routines throughout openFrameworks -- so drawing a ofRect(10,10,-10,-10);; yields the same output as ofRect(0,0,10,10);. Likewise drawing two rects defined the same way results in the same output. All that said, there are a lot of reasons to support negative w/h and most rectangles classes and rectangle drawing routines out there do.

Finally, the normalization/canonicalization functions are public: for convenience, but they are primarily used internally to make scaling calculations easier / clearer. The rect is currently "autoNormalized" during some scaling functions -- scaleFromCenter and scaleTo (which is the target of many overloaded scaling functions). So scaling functions always normalize a rect. This is a bit odd now that I think about it -- since it is the only time a rectangle gets autonormalized (is this problematic for anyone? It seems to be a little inconsistent ... and now that I'm thinking about it, I'm thinking it might be good to remove autonormalization and do the calculations manually ... thoughts?).

Anyway, with all of that said -- while would I still vote for the normalized nomenclature, I'd be on board with @DamianNZ or @ofTheo 's more self-descriptive nomenclature as long as we found a way to say

ofRectangle getNormalized() const

It basically needs to be a shorted version of this ...

ofRectangle getMeACopyOfThisRectangleAndMakeSureThatItsWidthAndHeightAreBothGreaterThanZeroEvenIfItMeansChangingTheValueOfXAndY() const

:)

@damian0815
Copy link
Contributor

@bakercp ok, that makes a lot of sense. i think your point around (x+width) == right is ok, even in light of the fact that .width, .height, .x and .y are all public.. i think we can live with it. arguments could be made for right to refer to either the minX or the maxX edge if the width is negative. i guess if you have negative width and/or height then you sort of know what you're doing wrt the 'right' edge of a rectangle. it's nice that ofRectangle has unambiguous functions like getMinX and getMaxX for these cases.

re: naming, my intuition for normalized would actually be that it would scale my rectangle so it has unit area with the same aspect ratio. how about like @ofTheo suggested:
makeDimensionsPositive // getMakeDimensionsPositiveised? getMakeDimensionsPositived?
or:
enforcePositiveWidthHeight // getEnforcedPositiveWidthHeight? getWithEnforcedPositiveWidthHeight?

re: scaling functions auto-normalizing: yes, i agree, this is a bit weird and/or inconsistent. scaling functions should not alter the sign of incoming width/height arguments.

@bakercp
Copy link
Member Author

bakercp commented Sep 30, 2012

Hi all -- sorry to keep this thread going -- but I just found that CGRect in Apple's ApplicationServices/ApplicationServices.h calls this "CGRectStandardize" and describes it the documentation here. @bilderbuchi suggested this very thing earlier in the thread.

Just to quote:

The height and width stored in a CGRect data structure can be negative. For example, a rectangle with an origin of [0.0, 0.0] and a size of [10.0,10.0] is exactly equivalent to a rectangle with an origin of [10.0, 10.0] and a size of [-10.0,-10.0]. Your application can standardize a rectangle—that is, ensure that the height and width are stored as positive values—by calling the CGRectStandardize function. All functions described in this reference that take CGRect data structures as inputs implicitly standardize those rectangles before calculating their results. For this reason, your applications should avoid directly reading and writing the data stored in the CGRect data structure. Instead, use the functions described here to manipulate rectangles and to retrieve their characteristics.

Would folks be opposed to this standardize() getStandardized() isStandardized()? If so, then we can just go with @DamianNZ's last suggestion.

I'm removing the auto normalization stuff from the scaling functions. Like the Apple Doc, we just need to be sure to mention that we don't enforce positive width / height and if users want to have predictable results, they should get a standardized copy or standardize the rectangle itself before using the other member methods like scaling, etc.

Thanks for all the feedback on this!

@ofTheo
Copy link
Member

ofTheo commented Sep 30, 2012

standardize() getStandardized() isStandardized()

  • sounds good to me!

@ofTheo
Copy link
Member

ofTheo commented Oct 1, 2012

merging this - would be good for people to test!!

thanks @bakercp for all the changes!

ofTheo added a commit that referenced this pull request Oct 1, 2012
Proposed update to ofRectangle::scaleTo - now with more scaling modes.
@ofTheo ofTheo merged commit 50781dd into openframeworks:develop Oct 1, 2012
@bakercp
Copy link
Member Author

bakercp commented Oct 1, 2012

Thanks for all of the input! For those interested in testing, please check out the new rectangleAlignmentAndScaling example in examples/geometry.

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

7 participants