Enhanced the Wave Effect, such that the "Y Position" of the wave is animateable

[davidmpeace]

I needed the Wave effect to raise up from the bottom for my sequence, so I decided to update the code to support this. I added a new animateable property to the wave effect called "Y Position"

This is my first pull request for X Lights, so if I did something out of process, please let me know.

[AzGilrock]

Can you explain why the multiply by 2.5 here? That seems like someone dials in a 100% deviation from center and it results in 250%.

int roundedWaveYOffset = -round(WaveYOffset * 2.5);

[davidmpeace]

Yes - I originally had the value without the multiplier (which would center the wave at the top or bottom perimeter). However, if you wanted to have the wave animate IN or OUT (which I did), it had to go out of frame. So considering the HEIGHT of the wave and the THICKNESS of the wave could be quite high, 2.5 was the number that could guarantee you could always get the wave off the boundary entirely if you needed to.

[AzGilrock]

I'm thinking it would be better to make the range of the slider go from -250 to +250 and then turn that into the percentage. That way if down the road we decide to make it -300 to +300 it won't impact existing sequences. If we later wanted to change the 2.5 multiplier to 3.0 you would have to do conversions on all the existing sequences.

[davidmpeace]

That makes a lot of sense. Thank you for the thoughtful reply - very helpful. I will make those updates, and hopefully be able to contribute more in the future as I get more familiar with the code.

[davidmpeace]

@AzGilrock I've made the update you requested. Please let me know if there's any other changes you'd like to see. I tried to keep with existing coding conventions as much as possible.

[AzGilrock]

Ok I get home late so not sure I'll get to try it. Maybe Keith will check it out though.

[keithsw1111]

I wont get a chance to look at this until much later ... but help me understand why the Y coordinate change cant be achieved using a change in the subbuffer ... I was hoping to remove not add x/y settings to individual effects.

[AzGilrock]

I know we had talked about trying to move all X/Y translation outside of the effects but I'm not sure how we do that unless we can feed that offset into the render routine of the effect instead of it being a post-render buffer translation which would cause black areas for the parts the render routine did not draw.

[keithsw1111]

Sure ... but we can also move the buffer itself across the model which has a similar result. Although I admit it is less obvious and requires 2 settings. For example to move the wave up 50% set the top of the buffer to 150 and the bottom to 50.

[AzGilrock]

I just can't tell if that handles the situation I'm thinking about. Best I can do is draw up what I'm thinking.

[keithsw1111]

To make it look like the middle one you need the buffer to be oversized.

[AzGilrock]

Well maybe we can create global X/Y translation options as long as it can figure out exactly how big to make the buffer to account for the shift.

[keithsw1111]

Ok ... but getting back to David's pull request ... do we add another effect with a Y coordinate awareness or can we reasonably suggest to a user that they move it around by playing with the subbuffer ... more inportantly by playing with the subbuffer can I generate the same outcome as this new parameter offers. If I can then I suggest dont add it. If we cant then I guess we add it.

At some point we do need to go back through this all and look at standardising things because it has become a bit of a dogs breakfast.

[AzGilrock]

I'd show him how to do it with the buffer and see what he thinks.

[davidmpeace]

hey guys - catching up on your conversation here. when you say "play around with the sub-buffer", I'm fairly certain it won't meet the need (unless I'm missing something).

I want to be able to move the wave from OFF screen (bottom) to on screen (middle), to give the effect of rising water.

I agree with Keith, having a setting in the Layer Settings sub-buffer manipulation side is optimal, because it would apply globally - but you can't do that today. Maybe I am misunderstanding.

Let me know

[keithsw1111]

If you were to set the sub buffer bottom to value curve ramp going from -100->0 and top to a value curve ramp from 0->200 doesnt that work?

[davidmpeace]

AH, just saw the right click "edit" option, where the Top Left Bottom and Right are animatable - that's awesome.

That being said, it doesn't work properly. It creates very odd stretching artifacts, and stray pixels. Maybe a bug that can be sorted?

[keithsw1111]

Can you package up your test sequence and I will take a look at it tonight.

[davidmpeace]

Sure thing - here you go.

Cheers!

Archive.zip

[keithsw1111]

This file does what i think you want.

TestWaveRising.zip

[davidmpeace]

Ahh I see what you did, and my mistake. Essentially, to get this behavior you have to animate both the TOP and BOTTOM parameters with identical movements. Frankly, while this does work, the panel is hard to find, and isn't the most straight-forward if someone was looking for the same type of effect.

To your previous point, it might make sense to add this as a main button in the Layer settings panel, so you can easily animate the X and Y position properties - to Gil's point, it WOULD clip if the original rendering went off screen.

How would you feel about me make this change, and submitting a new pull request?

It would do essentially the same thing as the right-click function - but bring it forward, and not require you to animate 2 properties identically to achieve it.

[AzGilrock]

Sorry but I need to hold up the STOP sign. If we are going to have a global X and Y offset I really think there needs to be a way that it can be fed into the effects not applied to the buffer after the effect is done rendering. Once we push features out we are usually stuck with them. Do we really want to later end up with a pre-render X/Y offset and a post X/Y offset? And I'm only asking developers.

[keithsw1111]

Ok ... so lets break it down.

#1 Do we need to add a Y offset to make the wave effect animate vertically ... no ... it can be achieved with the existing buffer value curves.
#2 Should we then move all x/y movement to depend on buffer value curves ... no ... as there will be effects where this is woefully inadequate.
#3 Should we have x/y as a global attribute and have some sort of consistency over effects ... personally I think we should but it opens up a can of worms as you would need to touch each and every effect ... and for some effects it makes no sense. So then you end up with an overridable functions on each effect bool SupportsX() and bool SupportsY() and a bunch of conversion logic to rewire all their settings to these new ones. I think this is probably the right thing to do ... but not in late September. I realise this seems slightly inconsistent with #1 but this is more about doing this right rather than just doing it. You then end up with the question of where to put the positioning parameters. They cant really stay on the effect window and they dont belong on the layer settings tab.

So my position ... lets not change it right now and revisit it after christmas more holistically.

[davidmpeace]

Agreed on all points. Would love to help when this is revisited.

Thank you guys for your help.

Closing.