-
Notifications
You must be signed in to change notification settings - Fork 54
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
Panning does not work if I use the margins property #80
Comments
Unfortunately no. Something went wrong with the margin property. WHen I tested it, it was fine but I get reports that it doesn't work as expected so maybe there is a bug |
@Fuzzyma thank you for your quick answer, I am going through the code and see if I can find a solution for that, can you explain to me why in the calculation of left, right, top and bottom you divide by the value of the zoom |
I wrote this code a while ago but I am pretty certain that dividing by zoom transforms the inner coordinates to outer coordinates. |
Thanks for the reply, but the problem that it doesn't work with zoom out, because the svg no longer moves throughout the didie poster area and even the margins are no longer good |
Salut @Fuzzyma I tried to fix the bug described in my previous post, indeed, following several tests made on different SVG types with different dimensions I added this fix for margins when zoom out. Before : After: What do you think of this fixed? if you agree I can create a pull request |
Thanks for taking the time. |
In my example I declared my panzoom like this |
Why exactly 0.5? Is it just some guess? An unprecise solution might work for your use-case but most likely not for others. |
If this is not the right way to do it, do you have any idea where the problem come from knowing that this necessarily linked to the restrictToMargins function which calculates the values of left, bottom, top and bottom? |
I would set a breakpoint and debug what's going wrong. I still think that the margin has to be divided by the zoom. |
Hi @Fuzzyma, I looked into this a bit more. The current code correctly restricts panzooming to a number of the outer pixels. For example, the top left corner of the svg can never be dragged past $margins.bottom/$margins.right pixels of the top left of the bottom right corner. This works for all zoom levels because the zoom is correctly taken into account in your maths. However, for my system, I don't want this behavior; instead, I want to restrict panzooming to a number of the inner pixels. That is, the $margins.bottom/$margins.right pixels of the top left part of the image at the initial zoom level can never be panzoomed outside the svg towards the bottom right. Here are codepens showing the two behaviors:
Here's the code change I had to do to get what I would like to have (not very easy to see in the codepen because I pasted the whole modified svg.panzoom.js file: - const leftLimit = width - left / zoom
- const rightLimit = (right - width) / zoom
- const topLimit = height - top / zoom
- const bottomLimit = (bottom - height) / zoom
+ const leftLimit = width - left
+ const rightLimit = right - width / zoom
+ const topLimit = height - top
+ const bottomLimit = bottom - height / zoom
+ I don't know if the margins I want should replace the margins that exist or if they should be separate options. To me, the way I want it feels a lot more natural (using the svg in the 2 pens, one feels arbitrary when zoomed in or out, the other feels natural). But maybe you have use cases where your way is more natural (I'd be interested to know about them out of curiosity) What do you think ? |
I like yours way more and I tend to think that this is how it was intended to work in the first place :D |
Great @Fuzzyma ! I just realized 2 things: const leftLimit = width - left
const rightLimit = right - box.width
const topLimit = height - top
const bottomLimit = bottom - box.height or (I like this better) with comments and zeroes added for symmetry (we could even put a simple ascii art drawing here..): const leftLimit = (width - left) - 0; // limit the right side of the svg (width) minus margin (left) to the left side of the box (0)
const rightLimit = (0 + right) - box.width; // limit the left side of the svg (0) plus margin (right) to the right side of the box (box.width)
const topLimit = (height - top) - 0; // limit the bottom side of the svg (height) minus top margin to the top side of the box (0)
const bottomLimit = (0 + bottom) - box.height; // limit the top side of the svg (0) plus bottom margin to the bottom side of the box (box.height)
So should we make the margins dependant on the initial viewbox width/height, instead of the svg outer width/height ? Having said that, how do you want to proceed ? |
Lets introduce some terms here:
It depends on what you call the margin. Is your margin measured in pixels or in user units? If its measured in pixels, what you actually wanna do is, take the 4 corners of the svg in px (0,0 400,0 400,400 0,400), apply the margin (-200,-200 600,-200 600,600 -200,00) and multiply them by the current zoom to transform them into userspace. However, if the margin is measured in user units (which makes more sense and that's what you have atm), you have to save the initial viewbox as you said. Then you can multiply the margins by the zoom from initial viewbox to current viewbox and calculate the limits: panZoom ({margin}) {
const viewbox = this.viewbox()
const restrictToMargins = box => {
if (!margins) return box
const { width, height } = box
let{ top, left, bottom, right } = margins
const zoom = viewbox.width / width
top *= zom
left *= zoom
bottom *= zoom
right *= zoom
const leftLimit = (width - left) - 0; // limit the right side of the svg (width) minus margin (left) to the left side of the box (0)
const rightLimit = (0 + right) - width; // limit the left side of the svg (0) plus margin (right) to the right side of the box (box.width)
const topLimit = (height - top) - 0; // limit the bottom side of the svg (height) minus top margin to the top side of the box (0)
const bottomLimit = (0 + bottom) - height; // limit the top side of the svg (0) plus bottom margin to the bottom side of the box (box.height)
box.x = Math.min(leftLimit, Math.max(rightLimit, box.x))
box.y = Math.min(topLimit, Math.max(bottomLimit, box.y))
return box
}
} Yeah, I guess I just repeated what you said :D |
I tested your code and it doesn't work for me ? panZoom ({margin}) {
const viewbox = this.viewbox()
const restrictToMargins = box => {
if (!margins) return box
const { width, height } = box
let{ top, left, bottom, right } = margins
const leftLimit = (viewbox.width - left) - 0; // limit the right side of the svg (viewbox.width) minus margin (left) to the left side of the box (0)
const rightLimit = (0 + right) - width; // limit the left side of the svg (0) plus margin (right) to the right side of the box (width)
const topLimit = (viewbox.height - top) - 0; // limit the bottom side of the svg (viewbox.height) minus top margin to the top side of the box (0)
const bottomLimit = (0 + bottom) - height; // limit the top side of the svg (0) plus bottom margin to the bottom side of the box (height)
box.x = Math.min(leftLimit, Math.max(rightLimit, box.x))
box.y = Math.min(topLimit, Math.max(bottomLimit, box.y))
return box
}
} |
Also, what about cases where the aspect ratio of the viewbox is not the same as the aspect ratio of the outer svg ? Then the drawing depends on the preserveAspectRatio property. I've made a bestiary of different possibilities, I'll try to make the computation work in all cases: https://codepen.io/jonenst/pen/yLarMXx?editors=1000 |
Also we should make it work for cases where the initial viewbox doesn't have its topleft corner at (0,0) ? This means changing to something like const leftLimit = (viewbox.width + viewbox.x - left) - 0; // limit the right side of the svg (viewbox.width+viewbox.x) minus margin (left) to the left side of the box (0)
const rightLimit = (viewbox.x + right) - width; // limit the left side of the svg (viewbox.x) plus margin (right) to the right side of the box (width)
const topLimit = (viewbox.height + viewbox.y - top) - 0; // limit the bottom side of the svg (viewbox.height + viewbox.y) minus top margin to the top side of the box (0)
const bottomLimit = (viewbox.y + bottom) - height; // limit the top side of the svg (viewbox.y) plus bottom margin to the bottom side of the box (height) (I like how this removes some 0 from the formulas; maybe the remaining 0 have to be replaced as well to correctly handle other edge cases) See it in action in this pen |
I am really happy that this issue is getting addressed or at least you are spending so much time on it. Please take a look at this Pen to see what is my logic, maybe you can use it on the library too! I would liked to give a PR, but I am hard set on deadlines in the project where I use your excellent library! Keep up the good work! |
Thanks for sharing, I will look into your code. For info, I'm working on a 3rd version of this code, I'm pretty sure now that I will have the correct code for all cases (the code becomes more complicated but not too much hopefully). I'll make a codepen when it's ready. Also, I would be in favor of swapping the left/right and top/bottom semantics if we go with user units (with the current code, left: 200 means that you want the 200 user units of the right side of the image to be always visible). Maybe we can also find a better word than margins for this. |
I've updated the bestiary pen @Fuzzyma with code that works in all cases: https://codepen.io/jonenst/pen/mdrgwoJ?editors=1000 @Fuzzyma Please tell me what you think and what you want to do. I can also change things if you want. I've kept the code as a single function to make it more easy to compare with the original code, but I think we should refactor this in multiple functions. Please note that leftLimit and topLimit are always the same, so they can be computed only once. And for rightLimit (resp. bottomLimit), the only dynamic part is adding box.width (resp. box.height) so most of the computation can be done only once. (I kept the whole computation in restrictToMargin to make it more easy to compare with the previous code, but we should change it). Keeping the same structure as your initial code, the new code is : const viewbox = this.viewbox()
const restrictToMargins = box => {
if (!margins) return box
let{ top, left, bottom, right } = margins
var _this$attr = _this.attr(['width', 'height', 'preserveAspectRatio']),
svgWidth = _this$attr.width,
svgHeight = _this$attr.height,
preserveAspectRatio = _this$attr.preserveAspectRatio;
const validPreserveAspectRatio = [
undefined,
"none",
"xMinYMin meet",
"xMidYMin meet",
"xMaxYMin meet",
"xMinYMid meet",
"xMidYMid meet",
"xMaxYMid meet",
"xMinYMax meet",
"xMidYMax meet",
"xMaxYMax meet",
"xMinYMin slice",
"xMidYMin slice",
"xMaxYMin slice",
"xMinYMid slice",
"xMidYMid slice",
"xMaxYMid slice",
"xMinYMax slice",
"xMidYMax slice",
"xMaxYMax slice",
]
if (!validPreserveAspectRatio.includes(preserveAspectRatio)) {
preserveAspectRatio = undefined;
}
// The current viewport (exactly what is shown on the screen, what we ultimately want to restrict)
// is not always exactly the same as current viewbox. They are different when the viewbox aspectRatio and the svg aspectRatio
// are different and preserveAspectRatio is not "none". These offsets represent the difference in user coordinates
// between the side of the viewbox and the side of the viewport.
let viewportLeftOffset = 0;
let viewportRightOffset = 0;
let viewportTopOffset = 0;
let viewportBottomOffset = 0;
// preserveAspectRatio none has no offsets
if (!(typeof(preserveAspectRatio) != "undefined" && preserveAspectRatio == "none")) {
const svgAspectRatio = svgWidth/svgHeight;
const viewboxAspectRatio = viewbox.width/viewbox.height;
// when aspectRatios are the same, there are no offsets
if (viewboxAspectRatio != svgAspectRatio) {
const changedAxis = svgAspectRatio > viewboxAspectRatio ? "width" : "height";
//aspectRatio undefined is like meet because that's the default
if (typeof(preserveAspectRatio) == "undefined" || preserveAspectRatio.includes("meet")) {
// in meet mode, the viewport is the viewbox, extended on one
// or both sides to match the aspectRatio of the svg
if (changedAxis == "width") {
const widthOffset = box.width/viewboxAspectRatio*svgAspectRatio - box.width;
//aspectRatio undefined is like mid because that's the default
if (typeof(preserveAspectRatio) == "undefined" || preserveAspectRatio.includes("xMid") ) {
viewportLeftOffset = -widthOffset/2;
viewportRightOffset = widthOffset/2;
} else if (preserveAspectRatio.includes("xMin") ) {
viewportRightOffset = widthOffset;
} else if (preserveAspectRatio.includes("xMax") ) {
viewportLeftOffset = -widthOffset;
}
} else {
const heightOffset = box.height*viewboxAspectRatio/svgAspectRatio - box.height;
if (typeof(preserveAspectRatio) == "undefined" || preserveAspectRatio.includes("YMid") ) {
viewportTopOffset = -heightOffset/2;
viewportBottomOffset = heightOffset/2;
} else if (preserveAspectRatio.includes("YMin") ) {
viewportBottomOffset = heightOffset;
} else if (preserveAspectRatio.includes("YMax") ) {
viewportTopOffset = -heightOffset;
}
}
} else if (preserveAspectRatio.includes("slice")) {
// in slice mode, the viewport is the viewbox, shrunk on one
// or both sides to match the aspectRatio of the svg
if (changedAxis == "width") {
const heightOffset = box.height - box.height*viewboxAspectRatio/svgAspectRatio;
if (preserveAspectRatio.includes("xMid") ) {
viewportTopOffset = heightOffset/2;
viewportBottomOffset = -heightOffset/2;
} else if (preserveAspectRatio.includes("xMin") ) {
viewportBottomOffset = -heightOffset;
} else if (preserveAspectRatio.includes("xMax") ) {
viewportTopOffset = heightOffset;
}
} else {
const widthOffset = box.width - box.width/viewboxAspectRatio*svgAspectRatio;
if (preserveAspectRatio.includes("YMid") ) {
viewportLeftOffset = widthOffset/2;
viewportRightOffset = -widthOffset/2;
} else if (preserveAspectRatio.includes("YMin") ) {
viewportLeftOffset = -widthOffset;
} else if (preserveAspectRatio.includes("YMax") ) {
viewportRightOffset = widthOffset;
}
}
}
}
}
// when box.x == leftLimit, the image is panned to the left,
// i.e the current box is to the right of the initial viewbox,
// and only the right part of the initial image is visible, i.e.
// the right side of the initial viewbox minus left margin (viewbox.x+viewbox.width-left)
// is aligned with the left side of the viewport (box.x + viewportLeftOffset):
// viewbox.width + viewbox.x - left = box.x + viewportLeftOffset
// viewbox.width + viewbox.x - left - viewportLeftOffset = box.x (= leftLimit)
const leftLimit = viewbox.width + viewbox.x - left - viewportLeftOffset;
// when box.x == rightLimit, the image is panned to the right,
// i.e the current box is to the left of the initial viewbox
// and only the left part of the initial image is visible, i.e
// the left side of the initial viewbox plus right margin (viewbox.x + right)
// is aligned with the right side of the viewport (box.x + box.width + viewportRightOffset)
// viewbox.x + right = box.x + box.width + viewportRightOffset
// viewbox.x + right - box.width - viewportRightOffset = box.x (= rightLimit)
const rightLimit = viewbox.x + right - box.width - viewportRightOffset;
// same with top and bottom
const topLimit = viewbox.height + viewbox.y - top - viewportTopOffset;
const bottomLimit = viewbox.y + bottom - box.height - viewportBottomOffset;
box.x = Math.min(leftLimit, Math.max(rightLimit, box.x)) // enforce rightLimit <= box.x <= leftLimit
box.y = Math.min(topLimit, Math.max(bottomLimit, box.y)) // enforce bottomLimit <= box.y <= topLimit
return box
} |
Thanks for the effort! My code was not tested and I only thought it out - guess I am not good at thinking :D. Yeah, lets include it. Your code however, can be shortened and simplified a lot. You dont need this "validPreserveAspectRatio" thing. Just let it default to |
Great ! Do you want me to do it or do you want to do it ? I can do it but I would say that the best choice would be for you to take it from here and use whichever code style you feel more comfortable with. And settle the questions of
ps: previously I said that 95% of the computation can be moved to the initialization phase, but your previous code did reread the width/height of the svg every the time, so to be feature compatible the preserveAspectRatio code should also be redone every time. I guess that's not much more than the previous computation so it's OK. ps: validPreserveAspectRatio was used to support user errors (for example preserveAspectRatio="xMaxYMax none", which is a plausible user error but in reality behaves exactly like preserveAspectRatio="xMidYMid meet"), but still allow easy testing of the preserveAspectRatio string (for example the code ps2: "guess I am not good at thinking :D." I think you did a fantastic job with this library, you don't have to be modest :) |
Yes, I will take it from here. However it would be cool if you could create a PR which so I have something to start with. |
Great, I will make a PR. Also, I found out that the browser exposes a validated preserveAspectRatio on the svg dom element, so no need to parse the html attribute. However, this revealed that preserveAspectRatio can be animated, see this pen: https://codepen.io/jonenst/pen/RwGXrrd :( . The previous code does work as is when using But this raises the following question: currently firefox and chrome only do discreet animations of preserveAspectRatio, but I don't see why it couldn't be animated smoothly in the future, so maybe we need to plan for that ? Using the current approach (where we compute the viewport offsets based on preserveAspectRatio), this would mean recomputing ourselves the interpolation of these viewport offsets, which I find unacceptable. So I think we would need to use an api of svg to get the viewport offsets (actually if we used that, it would make the code a lot simpler, I'll investigate) |
svg.js does not support SMIL animations. It has its own implementation for animations. So if the user comes across the (really rare) case of wanting to animate the aspect ration while also using svg.js (why would he in that case), its just bad luck. |
OK great. |
welp - you cant have anything ;) |
The accepted answer dates from 2014, so I added a comment asking if new APIs had been developed in 7 years... I'll wait a bit to see if there's a response, otherwise I'll make a PR with the current code. |
Yeah I saw your comment. Unfortunately browser vendors are notorously slow sometimes |
It's been a week, I've made a PR.. |
Yeah, I am not faster. Don't have much spare time atm |
I didn't mean "it's been a week, please hurry", I was answering to my own previous message " I'll wait a bit to see if there's a response", so I waited a week and made the PR :) |
Wasnt offended - no worries :D. Just wanted to say that it might take time to merge the pr :) |
Ok cool :) var style = window.getComputedStyle(this.node);
svgWidth = parseInt(style.width,10);
svgHeight = parseInt(style.height,10); instead of const { width, height } = this.attr(['width', 'height']) This allows margins to work even when the width/height of the svg element is not explicitly defined on the svg element attributes (for example width/height are "100%"). |
We had this before. This breaks when auto is used or percentage values. It is overall not really fault proof either. At some point I settled for simply pulling the values from the Dom and have this as precondition. I jist don't want 30 lines of code only for figuring our the dimensions of the svg |
Hi @Fuzzyma , can I help with anything to move this forward ? |
@jonenst yeah kick my lazy ass to do it lol 😄 |
any update on this? |
I am waiting for feedback on some code from @jonenst in the PR. So for a change, I am not the blocking factor :D |
I had missed your comments, sorry, I was waiting too ! I'll do it in a few hours |
You were right, the slice mode was totally broken.. see the fix: b432e49 Must have botched it when migrating to use the browser constants.. You can test the latest code here: https://codepen.io/jonenst/pen/mdrgwoJ?editors=1001 |
@Fuzzyma any update on this? |
@Fuzzyma are you waiting for something from me on this one ? |
@Fuzzyma is there anything I can do to help ? |
@Fuzzyma sorry to insist, but I really would like this fix to be merged at some point.. Anything I can do ? |
@Fuzzyma I saw your message in another issue "Please bear with me. I don't have much time to handle this atm" . I hope nothing too bad is happening to you. Would you be so kind to give an estimate of when you would have time to get this merged ? Thanks a lot. |
I am just really short on time with a few deadlines in my back. I hope to have some free time next month to handle this |
Hi @Fuzzyma , sorry to bug you again about this. It's been 2 months, can I do anything to get this merged ? |
@jonenst sorry. so busy... I answered in the pr |
Hello i have problem with panZoom i used the margins option to limit the pan area to at least 50px of my SVG still visible, but when i move the svg it disappears from the pan area and I don't see it anymore.
I'm using the margins option like this:
.panZoom({
panning: true,
zoomMin: 0,
zoomMax: 10,
zoomFactor: 1,
margins: {top: 50, left: 50, right: 50, bottom: 50}
});
Do you have any idea how i can fix it ?
The text was updated successfully, but these errors were encountered: