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

Added support for Backgroud Image #23

Merged
merged 5 commits into from
Nov 6, 2023
Merged

Added support for Backgroud Image #23

merged 5 commits into from
Nov 6, 2023

Conversation

dblood2
Copy link
Contributor

@dblood2 dblood2 commented Oct 25, 2023

Some of this may be too specific to my own project and not for users globally. Please feel free to edit my additions to make them better if necessary.

@dblood2 dblood2 mentioned this pull request Oct 25, 2023
@MarvinKlein1508
Copy link
Owner

Hi @dblood2

I've took a look on your PR and changed it a bit. Here are a few notes for you to understand what and why I changed it.

First of I've tried your implementation and I ended up in an infite refresh loop. So I took a look at your code and saw this:

        if (img.width > 0) {
            this._element.width = img.width;
            this._element.height = img.height;
        }
        else {
            if (image != "") {
                location.reload();
            }
        }

You should never, I repeat never force a reload in Blazor. This results in the server generating a new circuit for the user and the entire state of the app will be thrown away. In addtion the old circuit is still in memory due to the framework itself which keeps it for like 15 minutes.

Next I removed the two parameters for CanvasWidth & CanvasHeight and replaced them with blazors native feature to apply all extra parameters. This also makes it possible now to set apply all style through CSS, so I got rid of the hardcoded style="width:100%;" from my version.

I also got rid of adjusting the canvas size in the setBackgroundImage function. The size should only be set once. It's generally not a good idea to resize the canvas on the fly since this will override the last state of the canvas.

Since the background image should be loaded automatically everytime the image is being set, I got rid of all other calls to setBackgroundImage. Now I had it working on desktop.

So I checked my phone and it wasn't working there. The reason for this is that the window.devicePixelRatio is different depending on your resolution. So we need to draw the image based on the resolution of the device. So I've changed your drawImage implementation to this:

ctx.drawImage(img, 0, 0,
    img.width * window.devicePixelRatio,
    img.height * window.devicePixelRatio,
    0, 0,
    img.width * window.devicePixelRatio,
    img.height * window.devicePixelRatio
);

The last step I needed to do was applying the clientHeigth in the constructor. Then it also worked on my phone.

this._element.width = this._element.clientWidth;
this._element.height = this._element.clientHeight;

Could you checkout if this changes work for you as well?

I've tested them on a 1920x1080 monitor and on my iPhone 15 Pro.

@MarvinKlein1508 MarvinKlein1508 self-assigned this Oct 26, 2023
@dblood2
Copy link
Contributor Author

dblood2 commented Oct 26, 2023

I read through your comments, and I appreciate the detail you provided. I've pulled your changes and here are my thoughts:

  • As for the infinite loop and forcing a reload - point taken, but for me the original application did not load the image (nor a background color) on the initial load of the component without it. Not sure what's different about my environment or if the changes I'd already made caused this, but it just didn't. After pulling your changes, the background image loads just fine on the first load. It may have something to do with calling setImageBackground from within setImage which I discuss below.

  • On the height and width, I originally had done it on the actual canvas. I was overthinking it apparently by thinking that the app should size the component to the image dimensions automatically. I was trying to make it dynamic for no reason. Your fix is perfect and I learned something valuable there.

  • For the additional calls to setImageBackground, The reason that I was doing it that way is because the background image is merely the base image that gets loaded once, then the user draws on the canvas. In theory, having the call to setBackgroundImage within the setImage function (which runs on every edit) causes the changes to the canvas to be overwritten (setImage resets the canvas back to the background image). In my version, I was only setting the background image initially in the constructor, and when the clear() function was called.

The odd thing is that the reset doesn't happen on every call to setImage even though setBackgroundImage IS called. Again, I don't know if there's something afoul in my environment, or if you're seeing the same thing. I've recorded this and attached it to make sure you could see what I'm seeing.

Index.-.Google.Chrome.2023-10-26.10-39-23.mp4

@MarvinKlein1508
Copy link
Owner

@dblood2 hmm I'm not able to replicate this. Could you clear your browser cache and try again?

@MarvinKlein1508
Copy link
Owner

Nevermind. I found it. Could you check again?

@dblood2
Copy link
Contributor Author

dblood2 commented Oct 27, 2023

Seems to be working perfectly. Thanks again for the help.

@MarvinKlein1508
Copy link
Owner

I'm going to merge this next monday and all missing demos and update github pages

@MarvinKlein1508 MarvinKlein1508 merged commit 03f8b8c into MarvinKlein1508:master Nov 6, 2023
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