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

push v1.0 #44

Closed
wants to merge 2 commits into from
Closed

push v1.0 #44

wants to merge 2 commits into from

Conversation

HTV04
Copy link
Collaborator

@HTV04 HTV04 commented Aug 31, 2021

New PR: #46

Over the past few days, I spent some time rewriting and optimizing push, and present a potential v1.0 update:

  • Rewrote push to take advantage of local variables and functions instead of including non-API values in its own table. I also made it so that push no longer needs to call itself for functions, so push:function() is now push.function().
  • Simplified and extended push's setupScreen function, it no longer changes LOVE's window settings, but instead adapts to them.
    • New format is now: push.setupScreen(pushWidth, pushHeight, {upscale = ..., canvas = ...})
      • upscale (string): upscale push's resolution to the current window size
        • "normal": fit to the current window size, preserving aspect ratio
        • "pixel-perfect": pixel-perfect scaling using integer scaling (for values ≥1, otherwise uses normal scaling)
        • "stretched": stretch to the current window size
      • canvas (bool): use and upscale canvas set to push's resolution
    • Window width and height can still be overridden using push.resize(width, height).
    • This (technically) fixes Setting window width/height to 0 in push:setupScreen causes blank screen #40.
  • push now floors screen offsets internally to avoid drawing the screen in bewteen pixels.
  • push no longer creates a new table every frame when using the canvas setting, fixing memory usage steadily increases when using canvases #28 (and potentially Performance is terrible on Android if canvas flag = true #42?)
  • Removed push:setBackgroundColor(), since it was essentially just an integrated alias for love.graphics.setBackgroundColor().
  • Removed push:apply(), since it was pretty limited in functionality, only serving as an alias for push:start() and push:finish() (plus, API changes will require devs to change their code anyway).
  • Removed legacy LOVE 0.10 support.
  • Removed Mario assets from repo, because Nintendo (replaced with new LOVE-inspired pixel art).
  • Updated the README to showcase the new API, among other changes.
  • Updated low-res example.
  • Updated mouse-input example.
  • Updated main.lua, commented out unsupported examples for now.

An additional note: I think the canvas API is really cool, but it could use a little cleaning up and tweaks. Its code is mostly untouched in my rewrite, so it should work the same for now.

Also, I wanted to make the screenshot on the README a GIF like the original one, but I wasn't able to record one properly.

@HTV04 HTV04 requested a review from Ulydev August 31, 2021 04:15
@FormularSumo
Copy link

I can test to see if #42 is fixed tonight, and more generally test this rewrite. Sounds good though!

Copy link
Owner

@Ulydev Ulydev left a comment

Choose a reason for hiding this comment

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

Looking good, it's been a while I don't read Lua code haha
I think you could run your code through a prettifier to get consistent indentation / style.
If the canvas functionality is still working then the API change from : to . shouldn't make too much of a difference, if properly documented. Nice! I'm happy to see this library still being used and adapted to modern use cases. :-)

"mouse-input"
--[[
"canvases-shaders",
"stencil"
Copy link
Owner

Choose a reason for hiding this comment

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

Are the other examples not working? The more the merrier :-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, they should work, so I'll try to fix them up. I think I left them commented out because they used shaders, and I don't know if LOVE 0.10 shaders are compatible with LOVE 11. I'll try them out and let you know if they work okay.

push.lua Outdated Show resolved Hide resolved
@FormularSumo
Copy link

#42 is still a problem, FPS is unchanged from before when canvas == trues. However as canvas is set to false by default now it's not as much of a problem.

@FormularSumo
Copy link

To get my game running using this all I had to do was swap 'push:' with 'push.', add love.window.setMode, modify push.setupScreen and change mouseX == nil to mouseX == false, as push.toGame returns false now when mouse pressed outside in bezel area of window.

@HTV04
Copy link
Collaborator Author

HTV04 commented Oct 25, 2021

#42 is still a problem, FPS is unchanged from before when canvas == trues. However as canvas is set to false by default now it's not as much of a problem.

Hmm, weird. Are canvases usually slow on Android? If this is a LOVE issue I don't think I can work around this.

@FormularSumo
Copy link

FormularSumo commented Oct 25, 2021

#42 is still a problem, FPS is unchanged from before when canvas == trues. However as canvas is set to false by default now it's not as much of a problem.

Hmm, weird. Are canvases usually slow on Android? If this is a LOVE issue I don't think I can work around this.

I don't have much experiences with canvases but I just tried creating a project with 4 of them, rendering a 1920x1080 image to each and then drawing the canvases and FPS was locked at 60. Compare that to drawing just one 1920x1080 image using push.lua and canvas = true (3 canvases are created according to love.graphics.stats()) where FPS is 23. Test device is P20 Lite.

@HTV04
Copy link
Collaborator Author

HTV04 commented Nov 22, 2021

@FormularSumo Okay, so I've been thinking about ways to solve this, I believe what's causing this is the shader handling code.

I feel like the best solution would be to scrap the shader handling code entirely and let devs handle shaders since shaders will apply within the canvas anyway. This would probably fix the slowdown issue since the canvas feature would be reduced to simply rendering to a canvas instead of handling shaders for each frame. However, this would remove a known feature of push and I don't want to do that without @Ulydev's consent.

Another solution would be to refactor the shader code, but in order to optimize it the shader feature might need to be entirely reworked API-wise.

@idbrii
Copy link

idbrii commented Jan 9, 2022

I also made it so that push no longer needs to call itself for functions, so push:function() is now push.function().

Is this a good thing? When I see push.function(), I expect that push doesn't have any internal state whereas push:function() does.

However, usually I'd also see modules let users create their state object:

local push = require("push")() -- parens to trigger __call metamethod or fn that creates

That would follow what kikito wrote up about stateless modules.

setupScreen function, it no longer changes LOVE's window settings, but instead adapts to them

That sounds great!

push now floors screen offsets internally to avoid drawing the screen in bewteen pixels.

That sounds really interesting. I guess it maintains crisp pixels?

@HTV04
Copy link
Collaborator Author

HTV04 commented Jan 9, 2022

Is this a good thing? When I see push.function(), I expect that push doesn't have any internal state whereas push:function() does.

If you're talking about internal variables push uses for operation, yeah, it still has them. They are no longer exposed to the user within the push table though, internal variables are now local to push's functions.

Other than the fact that this change makes using : instead of . unnecessary, most libraries seem to be using ., including LÖVE's own modules, so I thought it would be neat for push to follow the same convention.

That sounds really interesting. I guess it maintains crisp pixels?

Yeah 😀

@HTV04 HTV04 closed this Jul 13, 2022
@HTV04 HTV04 deleted the proposed branch July 13, 2022 02:32
@HTV04 HTV04 mentioned this pull request Jul 13, 2022
Repository owner locked and limited conversation to collaborators Jul 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setting window width/height to 0 in push:setupScreen causes blank screen
4 participants