-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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 relative move and rotate to rel plugin #794
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR! You are addressing an area that I left behind when I had just started hacking on impress.js, because I felt it was too complex and I couldn't quite figure out how a user would be likely to use this. I just read this for the first time and made some comments while reading. I'll think about this for a bit and come back with more substantive comments later.
src/plugins/rel/rel.js
Outdated
// Once rotate-x/rotate-y/rotate-z is set, all three must be set or use default 0 | ||
var useRelativeRotate = data.rotateX === undefined && | ||
data.rotateY === undefined && | ||
data.rotateZ === undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not convinced this restriction is justified. If a user wants to set rotate-x="90" rotate-rel-y="45"
why should we forbid this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rel-rotate-*
is based on the status of previous slide, and rotate-*
is not, the final rotation may be applied to any axis depends on the rotation of previous slide. I can't image what's the propose and how to mix them.
If we allow the mixing, maybe we should finish the relative rotation, and then apply global rotation? Maybe it will looks like space distortion, just like we place objects in the plain space, and then distort the space. If we first add the angle and then do the rotation, I just can't image what the result is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an example, and this is how rel-x/y/z
work too:
<div class="step" data-rotate-x="45" data-rotate-y="45" >
<div class="step" data-rel-rotate-x="10" data-rotate-y="10" >
The effective absolute values for the second step are now rotate-x=55
and rotate-y=10
.
src/plugins/rel/rel.js
Outdated
relRotateZ: el.getAttribute( "data-rel-rotate-z" ), | ||
relPosition: el.getAttribute( "data-rel-position" ), | ||
rotateOrder: el.getAttribute( "data-rotate-order" ), | ||
relRotateOrder: el.getAttribute( "data-rel-rotate-order" ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the last one. How could there be a different rotate order for relative vs absolute? I don't think this last attribute is needed, or possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it's said in examples/3D-rotations/index.html, rotating order matters, relative rotating needs order too. It's ok to use the global data-rotate-order
, as we disallow mixing up rotate-x/y/z
and rel-rotate-x/y/z
, there's no need for two rotation-order in one slide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rotate order matters, but there can't be two orders effective at the same time.
Also, I'm not going to approve the proposal to disallow mixing relative and absolute coordinates. It's not how rel is designed to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late reply.
I've modified the plugin to allow mixing relative and absolute coordinates.
src/plugins/rel/rel.js
Outdated
step.z = step.z + rel.z; | ||
|
||
if ( useRelativeRotate ) { | ||
step.rotate = combineRotations( prev.rotate, step.relative.rotate ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this not just a sum like the above? What is all the math that follows good for?
Please add your reply as a code comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rotations in CSS is applied in order, and after each rotation, the rotation coordinate is updated accordingly. So we can't just sum up the angles. We need to know the new coordinate after each rotation, and calculate the position after the rotation according to it, and finally find out a one step rotation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you give some specific examples? I don't follow, and I just don't understand why this needs to be any more complicated than the example I've given in previous comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it's said in examples/3D-rotations/index.html
, order is matter.
For example, in my firefox, the following two slides
<div class="step" data-rotate-x="90" data-rotate-z="90" data-rotate-order="xyz">XYZ</div>
<div class="step" data-rotate-x="90" data-rotate-z="90" data-rotate-order="zyx">ZYX</div>
The XYZ one is horizontal and facing upside, the ZYX one is vertical and facing right side.
In my opinion, the XYZ one is first rotated by X-axis, facing upward, and then rotated by the Z-axis of its rotated coordinate, which is the negative Y-axis before rotation. So the result is just like two independent rotation, rotate-x=90 and rotate-y=-90. The ZYX one is similar, the rotated X-axis is the original Y-axis, so it works like rotate-z=90 and then rotate-y=90.
So after each rotation, we have to know what the X/Y/Z-axis is pointing to, and do the following rotations by them. So we can not simply sum up angles of one axis.
For example, rotate-x=90 then rotate-z=90 then rotate-x=90 NOT equals to rotate-x=180 then rotate-z=90.
Hope I explained it clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Furthermore, I suspect sum up the movement while data-rel-position="relative"
is also problematic. While data-rel-position="absolute"
, data-x
and data-rel-x
are using the same axis, so it can be sum up. But while data-rel-position="relative"
, data-x
and data-rel-x
may or maybe use the same axis, so it's not reasonable to be simply summed.
For example, <div class="step" data-x="100" data-rel-y="100">
, what's user's purpose?
If previous slide is at (0,0,0), and no rotation, the current slide will be x=100, y=100. If rotate-x=90 in previous slide, the effect will be x=0,y=0, just overlap with the previous one. If the user doesn't know the status of previous slide, how can he know the final status, how can he achieve what he need? If he knows, he should just use the absolute position.
So I think maybe we should disallow mixing up absolute and relative at all, while data-rel-position="relative"
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, order matters and that's why we have the ability to define rotate-order. This is how CSS works. It sounds like you're trying to work against this, and a) I don't quite understand why you're even doing this, and b) it leads to quite complicated code where you're essentially reverse engineering what CSS does in order to counter the rotation from normal CSS.
For example, rotate-x=90 then rotate-z=90 then rotate-x=90 NOT equals to rotate-x=180 then rotate-z=90.
Correct, they are not the same. What I'm trying to say is that rotate-x=180 is the correct choice.
If the user doesn't know the status of previous slide, how can he know the final status, how can he achieve what he need? If he knows, he should just use the absolute position.
Why would the user not know the position of the previous slide? It's the user who created the presentation.
The reason to use rel coordinates is not ignorance, but to save typing.
So I think maybe we should disallow mixing up absolute and relative at all
Again, it really sounds like you are trying to do something different than what is the point of the rel plugin. The rel plugin for sure is meant to mix well on top of the absolute coordinates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would the user not know the position of the previous slide? It's the user who created the presentation.
For easy preparing presentation, users may split the presentation into several groups, design each group independently, then finally assemble them into the final presentation. While they design one part, they should only care about the relative position, not the absolute position. The absolute position will be determined at the latest stage, by changed the position of first slide in a group.
For example, in examples/3D-positions/index.html
, the presentation is separated into three groups, one box and two rings. Two rings have the same shape, have the same code except for the first slide in each group. When I design them, I really don't know the final position of each slide. And all I need in final is to alter the position of the first slide, to get what I want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds like you're trying to work against this, and a) I don't quite understand why you're even doing this, and b) it leads to quite complicated code where you're essentially reverse engineering what CSS does in order to counter the rotation from normal CSS.
All I want is let the computer do more, the user do less.
When I design a presentation using the originally version of impress.js
, I met some problems. I want to separate the presentation in to several parts, in each part, main slides are laid out in a line, slide in from right, and sub slides are laid out vertical below the main slide, slide up one by one. It's easy, right? But while I want to change the direction of slides, trouble comes. E.g. when I want the three parts of slides form a triangle, slides in two of three parts should have an angle of ±60 degree. I found I have to carefully calculate the position of each slide, it is boring. So I thought, why not just tell the computer, I want the next slide at the east side of current slide, and have a distance of 1w? It's easy for computer to do this than we.
So I try finding a way to do this. After some search, I found that quaternion is the most recommended way, so I used it. It's a bit complicate, but I don't know whether there're simplier way.
Hi. Sorry for ignoring this great patch, I've been so busy at work this fall. I took a quick look and am starting to get what you're doing. Also your demo is really nice. Thanks for persisting. Are you ready from your side? I'll take a deeper look tomorrow/soon. |
Thx. I just want to make my life more eaier 😆 I don't have more for this function now. But I am not too good at math, some algorithm is developed by myself, I suspect that there may be some error in corner cases. For further improvement, if we allow specified the rotate center other than the center of slide, we can avoid calculate the center of next slide manually. It's not too hard to implement, but I don't know if we should put it into the rel plugin, or we should rework on the rel plugin, to add some extension point? |
I know. But what you have done is pretty brilliant now that I see it coming together. I couldn't figure this out 4 years ago when I developed the original rel plugin.
Ah right. I actually have an opinion on this. Thanks for reminding... The attribute ** But now that you bring this up, there's also #733. If we add sections, I could imagine that the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Speaking of data-rel-to
...
src/plugins/rel/rel.js
Outdated
prev.relative = {}; | ||
prev.relative.position = ref.getAttribute( "data-rel-position" ) || "absolute"; | ||
prev.relative.rotate = { x:0, y:0, z:0, order: "xyz" }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, why not? IMO both position and rotation should be relative to relTo slide. Basically it should be used 100% as if it's the previous slide.
3bf75ad
to
474fe47
Compare
src/plugins/rel/rel.js
Outdated
if ( data.relReset === "all" ) { | ||
inheritRotation = false; | ||
} | ||
} else if ( el.hasAttribute( "data-rel-inherit" ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this not the same as data-rel-to
?
I'm trying to remember if I have previously said something that is causing you to not modify that one? In that case I've forgotten, so please either remind me or explain why you would introcude a new attribute that is similar to an existing one?
For the newly added data-rel-rotate-z="32.73" data-rel-x="2059.55" data-rel-y="604.79" It's quite easy to specify those attributes for the second slide, and the following slides all inherit from it. But in one of those slide, I need to break the sequence by zoom in the slide, then zoom out and resume the sequence to form the circle. How can we resume the sequence? One way is use The This attribute is based on the real need, hope I make it clear. Maybe I can find some time to make an example. |
Thanks. I understand the explanation. Let me think about it for a while. It seems your other PR that was merged, now conflicts with this one for the tests. |
Ok, so I think what you are trying to do with
Now, later you want to "inherit" some attribute values from this slide to another slide:
This would copy all data-* attributes from slideone, but then set data-foo to 6. Does this make sense? |
The conflict is raised because both PR add new tests. I will updtae this PR to resolve it. |
Yes, it's what I want. But I need to clarify that:
|
Sorry for delay. Two weekends of snow blizzard have kept me busy outside.
Ok this is a good point. This makes this rather specific to rel plugin. Or even if there was a generic template plugin, it would need access to the internal state of rel plugin. To keep it simple, let's continue within the rel plugin and then if this becomes a popular thing, in the future someone can generalize it.
So let's probe this a bit more... In the normal case, both data-x and data-rel-x are inherited from the previous slide, and then of course the effective value of Now if you specify that position should be inherited from some other previous slide, why should that inheritance not work exactly the same? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing classic-slides changes.
Your work finally solves what I was thinking of when doing this demo. I'm so glad to see this.
Still have questions/thoughts about how we should design the syntax. No more comments on functionality I guess. We're really close now!
</div> | ||
</div> | ||
|
||
<div class="step slide" data-rel-x="1600" data-rel-y="1600" data-rotate="60"> | ||
<div class="step slide"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beautiful :-)
You've cracked what I wanted to do 5 years ago!
examples/classic-slides/index.html
Outdated
</div> | ||
</div> | ||
|
||
<div id="addons" class="step slide title" data-rel-to="motions" data-rel-inherit="motions"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would rel-to and rel-inherit ever be different?
Should we say that inherit implies also rel-to? I think we're missing something in the design here still, but I keep forgetting details we discussed months ago. Sorry.
I'm also thinking you introduced data-rel-position="relative"
mode. Would it make sense if the above line would instead just be:
<div id="addons" class="step slide title" data-rel-to="motions" data-rel-position="relative">
...and do exactly the same thing?
(Of course, data-rel-position could also be omitted, since it is itself inherited.)
I think my point here is that you are introducing a new attribute to keep existing syntax backward compatible, but data-rel-position="relative"
is already such new syntax where you can define behavior without worrying about backward compatibility.
Thanks.
I think the problem is we need TWO slide to begin a sequence: the first slide to position (it may has I think it's better to make a sequence in the SAME slide, then we can simplified the We need to think about that we need, and make a reasonable syntax. |
Thanks for the summary! Backward compatibility adds some complexity, but it's a necessary evil. IMO the fact that absolute/relative map clearly to the same options in CSS positioning makes this a nice interface to the use to digest. So I think everything else makes sense, and we should just continue to understand what to do with Also, you should ask yourself whether you are over optimizing? Using |
Just to keep the conversation going: What do you think about the KISS proposal: Just remote data-rel-inherit and merge its functionality with data-rel-to? |
Sorry for the late reply. I'm a bit busy and not have much time recentlly due to the COVID-19. I agree that we can remove When we remove |
The problem confuses me is what we should deal with |
Sorry to hear this. Don't apologize. This is a hobby project and my responses aren't always prompt either.
Since you've highlighted break from flow as a major use case, it seems you'd want to specify those anyway.
No, I don't see how that could make sense. NOT inheriting seems correct. |
Considering the original behaviours, I make the following changes when
The unittest is passed now. |
For the slide flow, for example, I have a series of slides, they all inherits the same relative attributes:
Some slide may need a detail explain, then return the the original slide. The detail slide is out-of-flow.
It may be hard to calculate the position of Slide4 based on Slide3.1, but it's easy to do based on Slide3, so it's better to inherit from Slide3, like the slide flow is not been interrupted. |
Otherwise all following slides would end up in the same position. (Same delta relative to data-rel-to slide.) |
So is this ready to merge now? |
Maybe you can check whether the old presentations works as expected, if so, I think it's ready. |
Good idea. They all look ok. |
Is your first comment still usable as a commit message? Or do you want to provide a commit message for the squashed commit? |
English is not my native language. You decide. I don't mind if you rewrite a new comment. |
Nor is it mine :-) I was more checking whether it's still accurate after changes. But I'll use it. Thanks. |
And there it is! Hey, I just wanted to come back and say THANK YOU. This is probably the coolest, most advanced patch I've seen since I started re-activating impress.js project. If you are interested to work on more stuff, please keep in touch and let me know if there's anything I can do to help? henrik |
Yes, I will keep using |
The relative position in rel plugin is currently based on the world coordinate. So for the same effect, like fly in from the right-hand side, we must use difference
data-rel-x
/y
/z
value. Why not let the plugin do the hard part?So I introduce a
data-rel-position
, when set torelative
, all relative attribute is based on the position and rotation of previous slide. So no matter the rotation of previous slide,data-rel-x="1000"
always looks like fly in from the right-hand side. We can change the position and rotation of one slide, and the position of all following slides will be changed too.When
data-rel-position
is set torelative
, relative rotation has a clear meaning. It describes the relative rotations between slides. We don't need to set rotations for all slide, setting the key slides is enough. Ifdata-rel-position
is notrelative
, the effect ofdata-rel-rotate-x
/y
/z
is not clear, so they're only used whendata-rel-position="relative"
.After the introduction of relative rotation, there're 6 attribute that will inherit from previous slide. If we want to set a relative X move, we have to set all other 5 attributes to 0. It's boring. So a
data-rel-clear
is used to set all 6 attributes to 0, and then the value specified in current slide is applied. I think this attribute should be used in the second slide of a sequence. The first one setup the basic position, the second set the increments, and the following ones inherit from the second slide.The
examples/3D-positions/indaex.html
shows some usage. As you can see, the html code of two slide ring is the same, and slides except for the first two in a ring has no position attributes. It work by inheriting the previous one.BTW, the
test/HOWTO.md
says the test js should be placed in the same directory of the plugin itself. But it doesn't point aout where to put the corresponding html file. But inkarma.conf.js
, onlytest/plugins/*/*.html
is served. So the html files should be put intotest/plugins/*
? Because I want test js put in the same directory of test html, I have to put the test js intotest/plugins/rel/
instead ofsrc/plugins/rel/
. Maybe we could update theHOWTO.md
to describe where to put html files?This PR invokes a lot math calculations. Basically, the rotation of a slide is translated into the coordinate describing the directions of X/Y/Z axes. And
data-rel-x
/y
/z
can be easily calculated by that. The rotations is the hard part, I mainly use the algorithm in the Quaternions and spatial rotation - Wikipedia to compose two and more rotations.I'm not a math guy, hope I don't make much mistakes.