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

Add Python backend and update README #71

Closed
wants to merge 6 commits into from

Conversation

hemangjoshi37a
Copy link

This commit introduces a new Python backend for enhanced image conversion
functionality and updates the README to reflect these changes.

New features:

  • Implement Flask-based Python backend for image to byte array conversion
  • Add support for horizontal and vertical scanning modes
  • Include preview image generation in the backend

README updates:

  • Redesign layout for improved readability and visual appeal
  • Add information about the new Python backend feature
  • Include screenshots of both Python and HTML applications
  • Update contributor list and improve credits section
  • Enhance overall formatting with emojis and better Markdown usage

Technical changes:

  • Add requirements.txt for Python dependencies
  • Create new app.py file for Flask application
  • Update index.html to work with new backend
  • Add result.html template for displaying conversion results

This PR aims to expand the project's capabilities and improve its
presentation to potential users and contributors.

added python implementation for this for API request type use-case
added screenshots
updated the design
@hemangjoshi37a
Copy link
Author

please add me to the contributors , i want to make it more elegant and functional (this repo.)

@javl
Copy link
Owner

javl commented Sep 9, 2024

Hi @hemangjoshi37a!

I'll need to find some time to take a proper look, especially for the Readme updates. Thanks for those!

After taking a quick look at your changes, I do have some questions, mostly: what problem do you think using Python in this way solves?

With my current version you can just open the index.html page in your browser and start converting your files. This works both online and locally if you download the files. (I do agree it could be more elegant and I've been working on this for a bit in the background already.)

Your version using Python seems to mostly add a lot of overhead: you need to have python installed with multiple dependencies (Flask, PIL and Numpy) to in the end get a simplified version of what the current tool can already do. In contrast; the only 'dependency' of the current version is a webbrowser.

I can imagine it being useful to some to have an img2cpp Python module that allows you to convert images, but in that case I'd expect it to be a proper module that you can import into your own projects, and for the Python script to also work standalone via commandline arguments (i.e. python img2cpp.py myImage.png --direction vertical --dithering floydsteinberg out.cpp). This is possibly related to what ironlungx is doing here: #69

So I'm wondering how you see this version being used, compared to the original approach of having a static website with just a .html and some Javascript?

@hemangjoshi37a
Copy link
Author

Hi there,
Actually my use case is if the user wants to use the python backend as api conversion tool then he can. It is an option along with your html page. it is not compulsory to use a python app . but it is there if you want.  I am developing one payment application in which i am converting image to byte array and i thought it would be a great integration for your project that is why i added this .

@hemangjoshi37a
Copy link
Author

in this change i think i broke something . can you please fix this ?
image

getting error like this in the chrome debug console

Uncaught TypeError: Cannot read properties of null (reading 'appendChild')
    at img.onload (script.js:847:25)

@javl
Copy link
Owner

javl commented Sep 9, 2024

I wrote a whole reply, but messed up and it didn't get posted, so lets try again...

The layout in that image does look a lot better already :) I'd really like to declutter the interface, but I'd also like not to clutter the code too much, which means I won't be merging the Python port at this point, as I feel it will add too much complexity without adding a lot of benefit at this point. I'd love to merge this once the Python part is fully fleshed out and has all the same functionality / can be used in the way I described before.

Some quick notes on some small 'problems' with the current PR. I really don't want to discourage you from adding to image2cpp, so please take this as constructive feedback :)

  1. The project is missing a requirements file for Python
  2. You've made Numpy a dependency by importing it, but it is never used
  3. For very large images the conversion is a lot slower (4+ seconds in Python vs 100ms in JS). Not a big deal since most people will use it with small images, but it would still be good to show some sort of progress indicator / block the 'generate' button while the script is working.
  4. I'm not sure how I feel about introducing all these emoji and HTML in README.md, which in my opinion should just stay in markdown. It does help to layout things a bit nicer though, so I might merge this later on (without the emoji)
  5. When making a PR like this it is often nice to open it against a separate branch, instead of your main branch. As it stands now every time you add something to main it will be added to this PR, which makes it large and harder to read. In my opinion it also helps to have smaller commits that describe what they do, instead of adding every change into a single, large commit.

Please do take a look at the https://github.com/javl/image2cpp/blob/main/LICENSE.md for image2cpp. Porting it to Python is perfectly fine, so is using it commercially, but you do need to link back to the original / give credit from projects where it is used, like your payment application. Also, when used in commercial projects, I always encourage to use the donate button ;)

@hemangjoshi37a
Copy link
Author

Sorry i am not interested in working with you. please dont consider this PR.

@javl
Copy link
Owner

javl commented Sep 10, 2024

I hope it's not because of my feedback?

@javl javl closed this Sep 11, 2024
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.

2 participants