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

WLED PixelArtCreator #3042

Merged
merged 17 commits into from
Jan 24, 2023
Merged

WLED PixelArtCreator #3042

merged 17 commits into from
Jan 24, 2023

Conversation

werkstrom
Copy link
Contributor

No description provided.

blazoncek and others added 5 commits January 18, 2023 22:56
- Dynamic & Dynamic Smooth
- Dissolve & Dissolve Rnd
- Juggles
- Game of Life
- Colorful
- Fireworks & Rain
* pulser bugfix: " % cols" was missing so the effect would simply run out of visible range
* float math: use optimized functions: sqrtf, fabsf
* two more comments where code could be optimized, but I'm not sure what is thecorrect solution
GoL mutations.
cleanup.
- full implementation
@blazoncek blazoncek changed the title Create file for PixelArtCreator WLED PixelArtCreator Jan 20, 2023
@blazoncek
Copy link
Collaborator

This is now properly implemented for inclusion with WLED including updated web server.
It has not been tested yet.

@blazoncek
Copy link
Collaborator

Modified to shave some bytes, adjusted the look to be more in-line with UI and fixed all inlining issues.
It can be edited and tested locally without issues.
The problem is that inlining WLED uses (nodejs inliner) strips ID tags from any SVG element so a different approach is needed.

@blazoncek
Copy link
Collaborator

After editing JSON in textarea field and sending the changed content to device, originally created JSON is sent instead, not the modified one.

Copy link
Collaborator

@blazoncek blazoncek left a comment

Choose a reason for hiding this comment

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

Please remove minified html dump from cdata.json
You may keep changes in your local file just don't include it in PR.

tools/cdata.js Outdated Show resolved Hide resolved
@blazoncek
Copy link
Collaborator

Better.

@blazoncek
Copy link
Collaborator

It looks like converter is no longer working for me when IP address is not entered/specified.
When I select/drop an image nothing happens. There is also no console error or other message displayed.
As you described earlier the JSON text should be displayed regardless of IP address (for copy purposes) but the send to device button should be hidden. The same could be made for CURL/HA but the IP address replaced by wled.local

@werkstrom
Copy link
Contributor Author

werkstrom commented Jan 23, 2023

Good input, your type of user is not the user I have in mind when building 😁 Some background and then suggested solutions to get you what you want.

It looks like converter is no longer working for me when IP address is not entered/specified.
Edit: I might change my mind on this one actually, since the JSON is valid without host. The CURL/HA/SEND is not. Will implement other behavior.
Expected behavior. Without IP/Host your generated code will not be valid. Also. IP/host will be missing as a default only in very specific use cases. It gets automatically populated from the host providing the file, i.e. the WLED device itself. Only if the host cannot be read, will it be empty. Solution proposed below.

the IP address replaced by wled.local
Edit: Same as above, Agree. Though a fringe case, it should be handled in the most easy to understand way. Still don't like wled.local as it implies a working host, that is not valid but in some cases could be valid. I'll start by implementing a fall back solution that can be checked when trying to use it (curl or ha generation or direct connection) and gracefully handled by the code.
Will only work if you have one WLED on your lan, and that is named wled.local or changes it manually in another place. Also likely to confuse users that delete the device name and try to use the code. An empty host name is per default a non valid setup. Solution proposed below.

There is also no console error or other message displayed.
Edit: To be clear, I do not disagree with you on the lack of feedback to the user. See solutions below.
Tool works as expected. Non valid use cases has been prevented. Since we check on every change of every property, we cannot log to console as it would be flooded. Input on solution requested below

Development use case explanation
As per today PixelArtConverter needs a minimum of clicks and input to work for the user that only wants to get images up to their device, i.e.:

Open PAC (possibly by clicking a link in WLED UI) --> Drag or select your file --> Click send.

I can see the use cases from a developer/testing/tinkering perspective, so any adjustments should be done in dev mode only.

Suggested short term solution: Manually add some character in the IP/host field and PAC will work exactly as you want it.

Long term (i.e. when I have time to sit down with this again, like in a day or two 😉)

  • Adding information to tell the user what is missing. Was planned but didn't have the time this weekend.
  • Quick check to see if the host is actually a WLED instance and in those cases download config/segments directly. This is not implemented today as I have not had the time to code a fast check. There are other scenarios where I'd like to implement such a check anyway to beter the user experience in some scenarios. Also planned and not done due to same reason.
  • In dev mode, JSON will get generated even without a valid setup. Not planned but I see your point.
  • Adding verbose logging to consol under some condition (dev or new "verbose_logging" setting). Not sure what is a good implementation of this. Input would be appreciated. Downside to more logging is the file size will increase. Suggest implementing this lastly to see what type of logging is actually needed.

I can see we have two very different, and conflicting use cases. The "everyday" user that is interested in getting things to just work, and the tinkerer/tester/developer that wants manual control, flexibility and insight into every detail. I can see both perspectives as valid. I've developed this tool mainly for the first category, but I think your suggestions makes sense in a dev-setting. I'll look into implementing your suggestions there. ASAP.

Please keep in mind: Any developer of WLED is most likely not the intended user of PAC. Rather the user of WLED is.

@blazoncek
Copy link
Collaborator

Haha, good point(s).
Once I figured out what is wrong (testing locally w/o ESP available) I managed to test validity and functionality (btw I try to test as a "dumb" user, but note every inconsistency or misbehavior).
I also did some code cleanup and reduction to shave about 100 bytes gzipped. If you agree I can push it back to this PR but you may not like every aspect of it. 😉

I would recommend to not add developer functionality (and possibly remove ?dev as well) to the final code. And TBH this already looks good for inclusion (merging). Instead of adding new features I would just try to clean the code and shrink it as much as possible without loosing readability.

@werkstrom
Copy link
Contributor Author

LOL... Changed my reply after some thought. TBH I totally agree on the dev thing once ready. If you are a dev, you could make your own copy of PAC locally and change the code to spit out anything you like. Comment on my edits if you disagree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants