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

Viewports #1504

Merged
merged 6 commits into from Mar 20, 2014

Conversation

@nooone
Copy link
Contributor

nooone commented Mar 20, 2014

I'm not sure whether I'm doing those PRs correctly or not, because somehow it seems like I'm always replacing whole files, but well, I'm new to git still...

I have written a little wiki page about the Viewports I've implemented. https://github.com/libgdx/libgdx/wiki/Viewports

There are also some tests. Every viewport now manages a camera. Mostly this was done to keep backwards compatibility and not break any API. I only had to add one more method to Stage which just sets the viewport parameters.

I like how it works now, the only thing I don't like is the automatic position change of the camera when Viewport.update() is called. This might be removed and be done manually instead, keeping it more clean in total (Viewport wouldn't do anything but implement a scaling strategy in this case), but it also means it's less comfortable to use.

Having that said, all changes are completely additionally. This doesn't "cleanup" the API, like I planned it initially, but it also doesn't break any core stuff...

@NathanSweet

This comment has been minimized.

Copy link
Member

NathanSweet commented Mar 20, 2014

Mario and I did a code review of your stuff:

Mario wants to remove the :D from the wiki, it's one of his pet peeves. :D Wiki is great otherwise.

Should have a direct link to tests at the bottom of the wiki. Too hard to find the viewport tests in the 140 tests we have.

Viewport shouldn't know about Stage. I see why you did it that way, but I think we can adjust Stage a bit so it isn't needed. This will make Viewport a little cleaner.

Since Stage is a high level class, I think it is fine to require the use of a Viewport to configure its Camera. Since the Stage will have the Viewport instance, it no longer needs to be updated.

The root table stuff I think can be removed. Someone who wants to do this can use a Viewport and call setBounds on their root table themselves. I think this use case is pretty rare anyway, I wrote that LetterBoxTest2 as a demo but have never actually used it in an app. We can show how to do it on the wiki page.

getPickRay(x,y,...) could be getPickRay(screenX,screenY,...) for better self documentation. We could also use project(worldCoords) and unproject(screenCoords) to make it clearer what needs to be passed in. Camera should be changed to match. FWIW, I prefer calling them "screen coords" rather than "window coords" and this matches the scene2d API, eg Stage#screenToStageCoordinates.

After giving some thought to enable/disable, I think it is better to remove them. If you want to draw in the black bars you can set glViewport to the whole screen, draw, then call Viewport#update(). The wiki can show this. I prefer this because there is less magic involved and Viewport doesn't need an "disabled" state where update() won't actually set the glViewport. It also means update(int, int) can be abstract and calculateViewport isn't needed.

We can merge as is if you want me to do these changes. Or if you can remove Stage stuff from Viewport then we can merge your stuff and I can refactor Stage (and all the examples and demos) to have a Viewport. Or if you'd like to do all the refactoring, that's fine with me too. :)

@nooone

This comment has been minimized.

Copy link
Contributor Author

nooone commented Mar 20, 2014

You guys are pretty quick when it comes to reviews ;)

Sorry about the ":D", it can be removed of course.

About the link to the tests... a direct link was difficult, because I cannot link to stuff which might be added in the future. ;)

I might do those small changes you suggested, but I guess that will save you 5 minutes at most. In total I would be really happy if you would take over this PR from now on and change it however you think is best. I once removed setViewport from Stage and was scared by how many tests would need to be updated/refactored in this case, because they all use Stage.

@NathanSweet

This comment has been minimized.

Copy link
Member

NathanSweet commented Mar 20, 2014

I had the review ready from when you went to bed on IRC a day or so ago. :)

Ok, will merge and see how it goes. Cheers!

NathanSweet added a commit that referenced this pull request Mar 20, 2014
@NathanSweet NathanSweet merged commit 0c31554 into libgdx:master Mar 20, 2014
@dsaltares

This comment has been minimized.

Copy link
Member

dsaltares commented Mar 20, 2014

I'm happy to supply DoubleRatioViewport and MinMaxViewport implementations once @NathanSweet is finished with the refactor. The Gemserk guys have the latter, which I'm using for my project anyway. It should be fairly straightforward to migrate.

NathanSweet added a commit that referenced this pull request Mar 20, 2014
@NathanSweet

This comment has been minimized.

Copy link
Member

NathanSweet commented Mar 20, 2014

I did some changes, all open to discussion of course. Stage has a Viewport, Viewport doesn't know about Stage. I added ExtendViewport, which is the old Stage#setViewport(w,h,true) behavior (scale to fit, then extend the world in one direction so no black bars). ScalingViewport does stretch, fit, fill, and none. Left StretchViewport, FitViewport and FillViewport just because it makes for a nicer API.

It's a little odd that the constructors that don't take a camera (only float worldWidth, float worldHeight) create an OrthographicCamera, but otherwise it makes using the API a bit annoying.

Feedback is welcome! Will do a blog post to help people with the transition. Stage API changed, scene2d apps need some minor fix up (sorry!).

@dsaltares

This comment has been minimized.

Copy link
Member

dsaltares commented Mar 21, 2014

Great stuff, I'm working on the DoubleRatioViewport and MinMaxViewport implementation. I'll probably commit later today.

@dsaltares

This comment has been minimized.

Copy link
Member

dsaltares commented Mar 21, 2014

Just added DoubleRatioViewport and MixMaxViewport along an abstract class MinMaxViewportBase where the shared code resides (to avoid duplication). I'll update the wiki ASAP.

@NathanSweet

This comment has been minimized.

Copy link
Member

NathanSweet commented Mar 21, 2014

Cool. I committed some minor cleanup.

What do you think about MinMaxViewport having an option to snap to the closest min/max size? Then we can get rid of MinMaxViewport or DoubleRatioViewport.

What do you think about MinMaxViewport having a list of sizes instead of just two? Maybe this is overkill. :)

@dsaltares

This comment has been minimized.

Copy link
Member

dsaltares commented Mar 21, 2014

Alright, stuff is in the wiki now.

I totally agree with adding the snap option, saves us quite a bit of code. However, the list of sizes might be a bit confusing. MinMaxViewport already indicates a continuous range of aspect ratios, it'd make little sense to use a list here.

On the other hand, it'd make sense to use a list in DoubleRatioViewport which could become MultipleRatioViewport but then, why not just use MinMaxViewport.

Going to add the snap option for now and update the wiki.

@NathanSweet

This comment has been minimized.

Copy link
Member

NathanSweet commented Mar 21, 2014

Yeah, the list was just a thought. I'd vote for a single class, MinMaxViewport, with snapping.

@dsaltares

This comment has been minimized.

Copy link
Member

dsaltares commented Mar 21, 2014

It is done now.

@NathanSweet

This comment has been minimized.

Copy link
Member

NathanSweet commented Mar 21, 2014

Is MinMaxViewport working like it should? What use case does it solve?

I think generally you would develop your landscape game to work correctly at a specific screen size, eg 640x480 (4:3). All UI controls and important things are within this area. Next you also make your game work at a wider screen size, eg 853x480 (16:9). Anything shown outside the 640x480 area is non-essential for gameplay. Now what you want is for your game to scale the 640x480 area to fit the screen, then instead of showing black bars on the sides, you show more of the world. Only if you go beyond 853x480 do you see black bars. This means you handle all aspect ratios between 4:3 and 16:9 (1.33 and 1.77). I'm not sure you'd ever want it to snap.

I tried this with MinMaxViewport:

int minWorldWidth = 640;
int minWorldHeight = 480;
int maxWorldWidth = 800;
int maxWorldHeight = 480;

Maybe I misunderstand its intent, but it doesn't seem to work like I would expect. When I slowly resize the window wider, the game's vertical scale changes. I'd expect the game to remain at a constant height. About halfway between the two sizes, the scaling jumps. I'd expect it to be smooth from one size to the other.

@dsaltares

This comment has been minimized.

Copy link
Member

dsaltares commented Mar 21, 2014

You could also have floating UI elements, that position themselves close to the edges of the current screen. Snapping might not be the desired behaviour for you, but would you ever use FillViewport?

I reproduced your problem with MinMaxViewport though. I'll work on a fix.

EDIT: it should be fixed now. I tried to keep the history clean but I had a conflict when merging your cleanup and didn't seem able to rebase it. What a newb 👎.

@xoppa

This comment has been minimized.

Copy link
Member

xoppa commented Mar 21, 2014

Not sure if it is intended, but I noticed that the default behavior of Stage is not the same as before. E.g. in my last tutorial (http://blog.xoppa.com/3d-frustum-culling-with-libgdx/) I used Stage to render fps, etc. The code regarding Stage is very simple: used the no-args ctor, no onresize, only add a label, and in render call stage.draw. This used to render the FPS at the lower left corner of the screen, but now it doesn't render anything at all. I added stage.getViewport().update(width, height); in the resize method, which caused the label to render at the center of the screen instead of the lower left corner. I haven't looked at it in depth, but the blog post doesn't mention this use case, so I though it might be worth noticing. BTW, what's the easiest method to place the camera at the lower left corner?

@dsaltares

This comment has been minimized.

Copy link
Member

dsaltares commented Mar 21, 2014

@xoppa AFAIK, you can position the camera manually wherever you want. The method to update the Viewport when upon resizing is:

public void update (int screenWidth, int screenHeight, boolean centerCamera);

You can pass false through so the camera position doesn't changed. Otherwise, it's centered in the origin.

@nooone

This comment has been minimized.

Copy link
Contributor Author

nooone commented Mar 21, 2014

Actually, when true is passed, it is not centered on the origin, but on the center of the virtual viewport.
Kind of like x+width/2, y+height/2. In that case the origin (0,0) will move to the bottom left corner of the screen (viewport).
That's what Stage did by default before when Stage.setViewport was called, so now it behaved differently.

@dsaltares

This comment has been minimized.

Copy link
Member

dsaltares commented Mar 21, 2014

My bad!

@NathanSweet

This comment has been minimized.

Copy link
Member

NathanSweet commented Mar 21, 2014

@siondream in MinMaxViewport is supposed to work like I explained, I don't think it does. I still see it scaling vertically between the min/max width using this:

int minWorldWidth = 640;
int minWorldHeight = 480;
int maxWorldWidth = 800;
int maxWorldHeight = 480;

If it is supposed to work a different way, please explain.

NathanSweet added a commit that referenced this pull request Mar 21, 2014
@NathanSweet

This comment has been minimized.

Copy link
Member

NathanSweet commented Mar 21, 2014

I added an optional max size to ExtendViewport. I believe this makes it do what we wanted MinMaxViewport to do: always fit the min size to the screen, then extend in one direction up to the max size. Is this right? Do we still need MinMaxViewport?

@dsaltares

This comment has been minimized.

Copy link
Member

dsaltares commented Mar 21, 2014

The problem appears in some situations within the interpolation case. That is, when the screen ratio is within the range. The code is as follows.

setScaling(Scaling.fill);
setWorldSize(maxWorldWidth, maxWorldWidth / screenAspectRatio);

Obviously, I shouldn't be taing maxWorldWidth as a reference in every situation. Sometimes, it might be better to scale using minWorldWidth, or something in between. I'll check with the pillow and take a look tomorrow.

@NathanSweet

This comment has been minimized.

Copy link
Member

NathanSweet commented Mar 21, 2014

I'm pretty sure ExtendViewport does everything you want and we no longer need MinMaxViewport. I added it to ViewportTest1/2/3, give it a try.

@dsaltares

This comment has been minimized.

Copy link
Member

dsaltares commented Mar 22, 2014

You're right, I'm sorry for the waste of time and all the bad committing. I removed MinMaxViewportentirely. Will update the wiki now.

@NathanSweet

This comment has been minimized.

Copy link
Member

NathanSweet commented Mar 22, 2014

No need to apologize, it just took a while to figure out the best way to go about it that handles all the cases.

@dsaltares

This comment has been minimized.

Copy link
Member

dsaltares commented Mar 22, 2014

Cheers. Just updated the wiki. We should probably write a blog post about this. I volunteer to write it though I don't have an account on the blog to publish.

@NathanSweet

This comment has been minimized.

Copy link
Member

NathanSweet commented Mar 22, 2014

Sure! @badlogic can give you access or send it to one of us.

@nooone

This comment has been minimized.

Copy link
Contributor Author

nooone commented Mar 22, 2014

When we are done, we should also write an entry for the CHANGES file.

Furthermore I accidentally deleted all my projects yesterday and couldn't recover anything. Epic Fail!

After recreating new projects with the gdx-setup-ui.jar and doing a GWT compile, I get these errors:
[ERROR] Line 38: No source code is available for type com.badlogic.gdx.utils.viewport.Viewport; did you forget to inherit a required module?
[ERROR] Line 51: No source code is available for type com.badlogic.gdx.utils.viewport.FitViewport; did you forget to inherit a required module?

Even when I comment all Viewport stuff out, Stage doesn't compile anymore as well, because the internally used Viewport seems to be missing.

[ERROR] Errors in 'com/badlogic/gdx/scenes/scene2d/Stage.java'
[ERROR] Line 62: No source code is available for type com.badlogic.gdx.utils.viewport.Viewport; did you forget to inherit a required module?
[ERROR] Line 78: No source code is available for type com.badlogic.gdx.utils.viewport.ScalingViewport; did you forget to inherit a required module?

On desktop it runs fine though and when I look at the referenced gdx.jar from the HTML project, I can see the Viewport classes.

Did we forget anything to include for the HTML backend?

@dsaltares

This comment has been minimized.

Copy link
Member

dsaltares commented Mar 22, 2014

@NathanSweet didn't realise you already posted on the blog explaining the changes!

@NathanSweet

This comment has been minimized.

Copy link
Member

NathanSweet commented Mar 22, 2014

@nooone Just fixed GWT. Probably.

@siondream Oh, yeah, I thought you wanted to post something additional. :)

I updated scene2d docs:
https://github.com/libgdx/libgdx/wiki/Scene2d
https://github.com/libgdx/libgdx/wiki/Scene2d.ui

@nooone

This comment has been minimized.

Copy link
Contributor Author

nooone commented Mar 22, 2014

@NathanSweet Yep, fix confirmed!

@dsaltares

This comment has been minimized.

Copy link
Member

dsaltares commented Mar 22, 2014

Do things like Box2DDebugRenderer or ShapeRenderer need to know about viewports? I integrated ExtendViewport into my game and the box2D debug graphics occupy the whole screen, even when black stripes appear.

Behold my placeholder graphics.

Fail

My code:

box2DRenderer.render(Env.game.getWorld(), camera.combined);

camera is obviously being updated by my ExtendViewport. Am I missing something?

@dsaltares

This comment has been minimized.

Copy link
Member

dsaltares commented Mar 22, 2014

Also, how would you go about having a world viewport and a UI one (the first using world units and the second one using pixels)?

When I only render the world or the UI everything seems fine.

UI only

However, when I render the UI on top of the world view, the UI doesn't scale properly. Similarly to what happens with the debug graphics.

UI and world

My code to create the viewports.

camera = new OrthographicCamera();
viewport = new ExtendViewport(800 * Env.pixelsToMetres, 600 * Env.pixelsToMetres, 1280 * Env.pixelsToMetres, 720 * Env.pixelsToMetres, camera);
uiCamera = new OrthographicCamera();
uiViewport = new ExtendViewport(800, 600, 1280, 720, uiCamera);

Resize...

viewport.update(width, height);
uiViewport.update(width, height, false);
uiViewport.getCamera().position.set(uiViewport.getWorldWidth() * 0.5f, uiViewport.getWorldHeight() * 0.5f, 0.0f);

The GlViewport should be the same, cause it should translate to the same screen dimensions.

@NathanSweet

This comment has been minimized.

Copy link
Member

NathanSweet commented Mar 22, 2014

Looks ok. Check the world sizes of each viewport to make sure they are what you think they should be. Try updating one viewport, drawing, update other, draw.

@dsaltares

This comment has been minimized.

Copy link
Member

dsaltares commented Mar 22, 2014

Thank you, this works.

  1. Update world viewport
  2. Render world
  3. Update UI viewport
  4. Render UI

The only downside is that I need to do it every frame. It does the job though.

@NathanSweet

This comment has been minimized.

Copy link
Member

NathanSweet commented Mar 22, 2014

I guess the glViewport calls in each viewport are different. Wonder why?

@dsaltares

This comment has been minimized.

Copy link
Member

dsaltares commented Mar 22, 2014

They seem to be the same though.

World viewport:

viewportHeight  427 
viewportWidth   911 
viewportX   184 
viewportY   0   
worldHeight 18.75   
worldWidth  40.0    

UI viewport

viewportHeight  427 
viewportWidth   911 
viewportX   184 
viewportY   0   
worldHeight 600.0   
worldWidth  1280.0  

I must be doing something else wrong. But no worries, the Viewportstuff seems okay.

@davebaol

This comment has been minimized.

Copy link
Member

davebaol commented Mar 23, 2014

Damn, I followed instructions posted on the blog and all my screens are fucked up now.
One thing I learned in almost 30 years programming is that backward compatibility is crucial.
Sorry guys, these API changes are getting me crazy.
:(

@NathanSweet

This comment has been minimized.

Copy link
Member

NathanSweet commented Mar 23, 2014

Sorry. Breaking shouldn't be done lightly, but being backwards compatible excessively makes for a horrible library (eg AWT or Swing). It's necessary for the library to improve.

It shouldn't take much to fix up. Once you get it right, all your screens will work.

@davebaol

This comment has been minimized.

Copy link
Member

davebaol commented Mar 23, 2014

Eh I already spent all the afternoon in vain trying to fix it up and I really suspect that I'll have to write a custom viewport.
The problem is that my app is a multilanguage word game supporting arabic too. And you know that libgdx doesn't support arabic scripting directly. So I have to use the underlying platform and move the gdxview to a different layout on screen transition. Also Gdx.graphics.width and Gdx.graphics.height can change from screen to screen.
I've already gone crazy with the old API to make it work properly, and now I'll have to do it again with the new one. That's why this API change is irritating me. :(

@NathanSweet

This comment has been minimized.

Copy link
Member

NathanSweet commented Mar 23, 2014

If you can explain how you want it to work or show how it used to work, maybe I can help.

@davebaol

This comment has been minimized.

Copy link
Member

davebaol commented Mar 23, 2014

Thanks, Nathan, I will. But first I have to get an idea of what the problem is. I've no clue right now.

@NathanSweet

This comment has been minimized.

Copy link
Member

NathanSweet commented Mar 23, 2014

You don't need to know what the problem is to describe how you want to scale your world size to the screen size.

@davebaol

This comment has been minimized.

Copy link
Member

davebaol commented Mar 24, 2014

Oh ok, I want a StretchViewport but it doesn't work as expected.
Well actually it only works for the splash screen, which is the 1st screen of course.
For all the other screens looks like the viewport works good only when game and display resolution are the same, i.e. 480x800. On a high resolution display the screen only takes 1/4 of the available space or doesn't appear at all.

@NathanSweet

This comment has been minimized.

Copy link
Member

NathanSweet commented Mar 24, 2014

The ViewportTest1/2/3 classes show StretchViewport works when the window is any size. Maybe you don't update in resize?

@davebaol

This comment has been minimized.

Copy link
Member

davebaol commented Mar 24, 2014

Yes, I do update on resize.
Also my app is much more complex than the tests mentioned.
And I can't write a simple test to reproduce my issue because lots of classes are involved to change the android layout and move the existing gdxview to it. Actually in my app each gdx screen has its own android layout.

EDIT:
ok problem solved :)
My screen transition framework does the following operations:

  1. shows an android black pane with an indeterminate progress bar
  2. inflates the new layout and moves the gdxview to it
  3. creates and shows the new gdx screen
  4. removes the black pane
    Inside one of the steps above there was a useless call to Screen#resize.
    I'm not sure why, but it was harmless with libgdx 0.9.9, while now it's ruinous with the last nightly build.
    After removing that useless call everything started working as expected and now I'm happy again :)
    Sorry for the hassle.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.