FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

push v1.0 by htv04 · Pull Request #44 · Ulydev/push · GitHub

/ push Public

push v1.0 - #44

Closed
htv04 wants to merge 2 commits into
masterfrom
proposed
ConversationCommits (2)ChecksFiles changed
Closed

push v1.0#44
htv04 wants to merge 2 commits into
masterfrom
proposed

Conversation

htv04 commented Aug 31, 2021 •
edited
Loading

Copy link
Copy Markdown
Collaborator

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 requested a review from Ulydev August 31, 2021 04:15

FormularSumo commented Aug 31, 2021

Copy link
Copy Markdown

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

Ulydev reviewed Aug 31, 2021
View reviewed changes

Ulydev left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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. :-)

Comment thread main.lua
"mouse-input"
--[[
"canvases-shaders",
"stencil"

Ulydev Aug 31, 2021

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

htv04 Oct 25, 2021

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread push.lua Outdated

FormularSumo commented Aug 31, 2021

Copy link
Copy Markdown

#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 commented Aug 31, 2021

Copy link
Copy Markdown

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 commented Oct 25, 2021 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

#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 commented Oct 25, 2021 •
edited
Loading

Copy link
Copy Markdown

#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 commented Nov 22, 2021 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

@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 commented Jan 9, 2022

Copy link
Copy Markdown

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 commented Jan 9, 2022 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

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 closed this Jul 13, 2022
htv04 deleted the proposed branch July 13, 2022 02:32
htv04 mentioned this pull request Jul 13, 2022
Draft
Repository owner locked and limited conversation to collaborators Jul 13, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

Ulydev Ulydev left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

4 participants

Back | FazBrowse Home | New Git URL