melonjs / melonJS

a fresh, modern & lightweight HTML5 game engine
https://melonjs.org
MIT License
5.88k stars 646 forks source link

Fix nodeBounds calculation in onresize() #1187

Closed johnhyde closed 1 year ago

johnhyde commented 1 year ago

It used to use the dimensions of the parent of the parent of the game, when it should just be the parent. Now we just get the element bounds of the parent element.

obiot commented 1 year ago

this changes actually breaks the resizing features, try to run the platformer example, resize the window and you will see the game canvas does not resize properly anymore.

for me, using device.getParentBounds() is correct as here we are trying to get the maximum canvas size within the parent div containing the canvas container (so there is a parent of parent notion)

what were you trying to fix ?

johnhyde commented 1 year ago

I wasn't running the example, I just know that when I had a #game div set to 500x500, an autoscaling canvas would scale based on the parent of the #game div, which meant overflowing my div. In my mind, the div I provide to render the game in should define the maximum bounds of the rendered game. game.getParentElement() returns my #game div which I specified in me.video.init. Getting the parent of that returns my #root div, which I don't think the game should know anything about.

johnhyde commented 1 year ago

In the platformer example, it would work fine if the screen was set to take up the full window. It just seems weird to me that the size of the main container div is ignored, and I need to actually define its bounds with another div outside that. This would be a major breaking change I suppose. Maybe as a compromise the documentation could explicitly specify which element the canvas will be autoscaling to? Also, I misunderstood the comment you quoted, thinking that the "canvas container" was the canvas. Maybe it could be revised: // get the maximum canvas size within the parent of the div containing the canvas Sorry for creating a PR before I really understood what was going on.

obiot commented 1 year ago

no no, please there is no need to be sorry, at the opposite, thank you very much for the pull request and for contributing !

and to be honest, I personally never use nested or multiple divs. Most, not to say all, my games are fullscreen, so it never really did bother me. The only time I used multiple div was for the UI example (see https://github.com/melonjs/examples/blob/master/UI/index.html), and same it works for me.

so I'm not against changing things, maybe as you said it could be an option to specify on which element it should be autoscaling (default being the current one, but else and enable to specify a div name ?)

obiot commented 1 year ago

Hi @johnhyde, in the last commit here (https://github.com/melonjs/melonJS/pull/1187) I added a new scaleTarget option as discussed that allows defining which HTML element should be used as "reference" when using auto scaling.

would you be able to pull the changes on your side, give a try, and adjust it if necessary to what you are looking for ?

johnhyde commented 1 year ago

Yep, using scaleTarget gives me the result I was looking for, thank you

obiot commented 1 year ago

awesome ! Unless you have other pull requests or requests, I will publish version 15.3 later so that you can keep going on your side with your project.

johnhyde commented 1 year ago

Ok, I'm not blocked, so no rush. The workaround of adding another container div was easy to do once I knew to do it.

obiot commented 1 year ago

15.3 published ! when you get something ready to show, come share some screenshots on discord :)