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

fix POV Display usermod #4427

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

Liliputech
Copy link

There was a compile issue when trying to enable POV Image effect.
This small PR fixes the problem.

@DedeHai
Copy link
Collaborator

DedeHai commented Dec 25, 2024

did not try to compile, but generally looks ok to me.

@softhack007 softhack007 added the usermod usermod related label Dec 28, 2024
@softhack007
Copy link
Collaborator

@Liliputech please do not force-push while you have a PR open for the the main WLED repo. It causes lots of crazy side-effects like lost review comments, auto-closed bug reports and broken links to code authors.

Liliputech force-pushed the pov_display branch from 3402a05 to 001b882 Compare3 days ago

@softhack007 softhack007 self-requested a review December 28, 2024 14:03
@softhack007 softhack007 added the in progress This pull request is currently being reviewed and, if necessary, modified label Dec 28, 2024
@softhack007
Copy link
Collaborator

softhack007 commented Dec 28, 2024

@Liliputech I think the usermod needs some more fixing and improvements.

  • I've tried to compile it with our default esp32dev environment - even with your patches, compilation just fails inside the PNGdec library (bitbank2/PNGdec@^1.0.1).
    Reverting to bitbank2/[email protected] (i.e. pinned to version 1.0.1) allows me to successfully build.
  • The usermod really needs a readme.md file to explain how to use it
  • I like your effort to also make the usermod compatible with "MoonModules" style usermods :-)
  • I'm wondering why f and png and some other functions are declared globally, with names that simply "invite trouble".
    Normally everything should be defined inside the usermod class, unless there are very strong reasons to leave the class scope.
  • the line buffer in the draw function (uint16_t usPixels[SEGLEN]) seems much too large, which is another case of "invites trouble"
  • Do you know why setPixelColor is used, and not setPixelColorXY ? Is this POV usermod intended for 1D strips, or for 2D setups?

Btw, do you know who is "Arthur Suzuki"? he appears as the author of initial commit, but does not have a github account. He has directly committed the usermod into the Aircoookie repo, so he must be a maintainer? or maybe just a hacker who abused a security flaw in github?

@Liliputech
Copy link
Author

@Liliputech I think the usermod needs some more fixing and improvements.

Hi @softhack007 Thank you for your feedback :)
I'll make the requested changes as soon as I can (probably by the end of next week, to give a good start to 2025!)

* I've tried to compile it with our default esp32dev environment - even with your patches, compilation just fails inside the PNGdec library (bitbank2/PNGdec@^1.0.1).
  Reverting to bitbank2/[email protected] (i.e. pinned to version 1.0.1) allows me to successfully build.

Maybe I should try to fix that by updating my code to fit the new version of PNGDEC instead then.

* The usermod really needs a readme.md file to explain how to use it

Sure! And a couple pictures!

* I like your effort to also make the usermod compatible with "MoonModules" style usermods :-)

Glad you liked it :)

* I'm wondering why `f` and `png` and some other functions are declared globally, with names that simply "invite trouble".
  Normally everything should be defined inside the usermod class, unless there are very strong reasons to leave the class scope.

Got to fix this indeed.

* Do you know why `setPixelColor` is used, and not `setPixelColorXY` ? Is this POV usermod intended for 1D strips, or for 2D setups?

It is intended for 1D strip, the idea is to have an imaged displayed on a rotating strip.

* the line buffer in the draw function (`uint16_t usPixels[SEGLEN]`) seems much too large, which is another case of "invites trouble"

Unfortunately there is not much to be done here if you want to properly decode PNG.
The GetLineAsRGB565 from PNGDEC need to store each pixel of a line in an int16, and the whole line in an array of the corresponding length (picture width).
The idea is to have a whole line of pixel displayed on the strip. then another, etc...
If the strip is moving fast enough and given the image width equals the strip length, you can get floating images in the air, like an hologram.

Btw, do you know who is "Arthur Suzuki"? he appears as the author of initial commit, but does not have a github account. He has directly committed the usermod into the Aircoookie repo, so he must be a maintainer? or maybe just a hacker who abused a security flaw in github?

I am Arthur :) you can find the information elsewhere by searching for my nickname or profile picture ;)
AFAIK I'm not allowed to push straight or merge to Aircookie repo... Feel free to PM me if needed, I'd be happy to discuss with fellow WLED hackers! (sometime do when connected to discord, same nickname)
Have a nice 2025!
Arthur

@dosipod
Copy link
Contributor

dosipod commented Dec 31, 2024

@Liliputech Since you are fixing things might be consider adding a readme as it is strange the usermod even got merged without one

@Liliputech
Copy link
Author

Actually I'm thinking about re-using the routines provided by the "image_loader.cpp" file provided by the GIF decoder effect. That would make a lot more sense :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in progress This pull request is currently being reviewed and, if necessary, modified usermod usermod related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants