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

Bug: BR_SetMediaItemEdges should keep envelope points in place #950

Closed
X-Raym opened this Issue Feb 12, 2018 · 22 comments

Comments

Projects
None yet
3 participants
@X-Raym
Copy link
Contributor

X-Raym commented Feb 12, 2018

Hi,

The ReaScript function BR_SetMediaItemEdges keep envelope points position relative to the item left edges, instead as leaving them at their absolute position in the project (the whole point of this function is to modify items and by preserving absolute position of the content, so envelope points should move).

Steps:

  1. Have an item
  2. Create an envelope
  3. Add points
  4. use the SetMediaEdges function
  5. see that envelope has been offset.

demo

Thanks for taking a look :)

@X-Raym

This comment has been minimized.

Copy link
Contributor Author

X-Raym commented Feb 12, 2018

Some Lua code that may help

function OffsetTakesEnvelopes(take, offset)
  local env_count = reaper.CountTakeEnvelopes(take)
  local take_rate = reaper.GetMediaItemTakeInfo_Value( take, "D_PLAYRATE" )
  for i = 0, env_count - 1 do
      local env = reaper.GetTakeEnvelope(take, i)
      local env_points = reaper.CountEnvelopePoints( env )
      for j = 0, env_points - 1 do
        local retval, time, value, shape, tension, selected = reaper.GetEnvelopePoint( env, j )
        reaper.SetEnvelopePoint( env, j, time + offset*take_rate, value, shape, tension, selected, true )
      end
    reaper.Envelope_SortPoints(env)
  end
end
@cfillion

This comment has been minimized.

Copy link
Collaborator

cfillion commented Feb 12, 2018

Same with "SWS/BR: Trim MIDI item to active content" which uses the same code as BR_SetItemEdges. The code is also used for some tempo-related actions, not sure if moving envelope points would affect those.

The code in question:

sws/Breeder/BR_Util.cpp

Lines 1081 to 1126 in b3ad441

bool TrimItem (MediaItem* item, double start, double end, bool force /*=false*/)
{
if (!item)
return false;
if (start > end)
swap(start, end);
if (start < 0)
start = 0;
double newLen = end - start;
if (newLen <= 0)
return false;
double itemPos = GetMediaItemInfo_Value(item, "D_POSITION");
double itemLen = GetMediaItemInfo_Value(item, "D_LENGTH");
bool itemLooped = !!GetMediaItemInfo_Value(item, "B_LOOPSRC");
MediaItem_Take* activeTake = GetActiveTake(item);
if (force || start != itemPos || newLen != itemLen)
{
double startDif = start - itemPos;
SetMediaItemInfo_Value(item, "D_LENGTH", newLen);
SetMediaItemInfo_Value(item, "D_POSITION", start);
for (int i = 0; i < CountTakes(item); ++i)
{
MediaItem_Take* take = GetTake(item, i);
double playrate = GetMediaItemTakeInfo_Value(take, "D_PLAYRATE");
double offset = GetMediaItemTakeInfo_Value(take, "D_STARTOFFS");
SetMediaItemTakeInfo_Value(take, "D_STARTOFFS", offset + playrate*startDif);
if (IsMidi(take))
{
SetActiveTake(take);
if (!itemLooped)
MIDI_SetItemExtents(item, TimeMap_timeToQN(start), TimeMap_timeToQN(end)); // this will update source length (but in case of looped midi item we don't want that (it also disabled looping for item)
}
}
SetActiveTake(activeTake);
return true;
}
return false;
}

@nofishonfriday

This comment has been minimized.

Copy link
Collaborator

nofishonfriday commented Feb 15, 2018

Potential fix for testing (Win 64):
https://www.dropbox.com/s/6no74lzkbu13pk8/reaper_sws64.dll?dl=1

Should hopefully fix both, BR_SetItemEdges() and "SWS/BR: Trim MIDI item to active content".
Testing and feedback appreciated, if it works I'll do a PR.

@X-Raym

This comment has been minimized.

Copy link
Contributor Author

X-Raym commented Feb 15, 2018

@nofishonfriday Tested with various item and playrates and it seems to work !!

Demo

Many thanks !

@nofishonfriday

This comment has been minimized.

Copy link
Collaborator

nofishonfriday commented Feb 15, 2018

Nice, thanks for the quick feedback.
While you're at it, could you also test if it still works correctly if takes contain stretch markers (with rate other than 1.0) ?
(I have a fear it could break then, didn't dare to test myself yet :P)

edit:
Oh and thanks for the Lua code, it helped. :)

@nofishonfriday

This comment has been minimized.

Copy link
Collaborator

nofishonfriday commented Feb 16, 2018

Previous version did break with SM's here, as I thought.
Hopefully also fixed in new version, please test again (with SM's):

https://www.dropbox.com/s/bsh18aqg0bjs3q8/reaper_sws64.dll?dl=1

@X-Raym

This comment has been minimized.

Copy link
Contributor Author

X-Raym commented Feb 16, 2018

@nofishonfriday

It works in most case, but there is some issues:

Demo

(item on top would be the expected behavior, it is the result we have if we extend items manually)

Here is a demo project.

https://www.dropbox.com/s/pm7nqcqoqh1gmd1/BrSetItemEdges.zip?dl=0

reaper.Undo_BeginBlock()
item = reaper.GetSelectedMediaItem(0,0)
if not item then item = reaper.GetMediaItem(0,0) end
reaper.BR_SetItemEdges(item, 10,30)
reaper.Undo_EndBlock("haha", -1)```

EDIT: the problem is that SM don't stay in place.
@X-Raym

This comment has been minimized.

Copy link
Contributor Author

X-Raym commented Feb 16, 2018

Also, there is some minor problem with Snap Offset in very particular cases

demo

By hand with click and drag, snap offset stay in place.

In this case (item has some sort of incomplete pre-source extension), snap offset isn't preserve at the right absolute position.

EDIT: it seems that it is just the item snap offset which isn't updated. Simply as that :P

@X-Raym

This comment has been minimized.

Copy link
Contributor Author

X-Raym commented Feb 18, 2018

@nofishonfriday
I asked to Stretch Marker master MPL to take a look and he got pretty successful results !! 👍

Demo

reaper.Undo_BeginBlock()

item = reaper.GetSelectedMediaItem(0,0)

if not item then item = reaper.GetMediaItem(0,0) end



take = reaper.GetActiveTake(item)

pos1 = 0

pos2 = reaper.GetMediaItemInfo_Value( item, 'D_LENGTH' )* reaper.GetMediaItemTakeInfo_Value( take, 'D_PLAYRATE' )

reaper.SetTakeStretchMarker( take, -1, pos1 )

reaper.SetTakeStretchMarker( take, -1, pos2 )

it_pos = reaper.GetMediaItemInfo_Value( item, 'D_POSITION' )

t = {} for idx = 1,  reaper.GetTakeNumStretchMarkers( take ) do 

  local ID = #t+1

  t[ID] = {}

  _, t[ID].pos, t[ID].srcpos = reaper.GetTakeStretchMarker( take, idx -1)

  t[ID].slope = reaper.GetTakeStretchMarkerSlope( take, idx -1)

end



reaper.DeleteTakeStretchMarkers( take, 0, 10000000 )

reaper.BR_SetItemEdges(item, 10,30)

it_pos2 = reaper.GetMediaItemInfo_Value( item, 'D_POSITION' )

for i = 1, #t do  

  local idx = reaper.SetTakeStretchMarker( take, -1, t[i].pos+(it_pos-it_pos2)* reaper.GetMediaItemTakeInfo_Value( take, 'D_PLAYRATE' ), t[i].srcpos )

  reaper.SetTakeStretchMarkerSlope( take, idx, t[i].slope )

end

reaper.Undo_EndBlock("haha", -1)

reaper.UpdateArrange()

Very close to what we get with manual extension (tough, the audio doesn't loop here before the stretch markers for some very weird reason. But the solutions should be close to MPL code).

(thanks again to MPL !!)

Here are some debug

Original file with manual extension
0 pos= 9.017691156 src= 0.0
1 pos= 9.017692156 src= 1e-006
2 pos= 10.011767347 src= 0.52767491
3 pos= 11.786903401 src= 1.419655986

New file
0 pos= 9.017691156 src= 0.0
1 pos= 9.017692156 src= 1e-006
2 pos= 9.0886965986394 src= 0.037691500893791
3 pos= 10.011767347 src= 0.52767491
4 pos= 11.786903401 src= 1.419655986
5 pos= 13.633044897959 src= 3.265797482959

Seems like there is extra SM with MPL solution (SM 2 and 5). Maybe it is related.

@X-Raym

This comment has been minimized.

Copy link
Contributor Author

X-Raym commented Feb 18, 2018

@nofishonfriday Oh man, I found a very quick way to do it !!

demo

The BR_SetItemEdges could be rewritten just with edit cursor, and these trim actions !!

@nofishonfriday

This comment has been minimized.

Copy link
Collaborator

nofishonfriday commented Feb 18, 2018

The BR_SetItemEdges could be rewritten just with edit cursor, and these trim actions !!

Doh, yes, this seems to work in this case. Needs to be careful though not to break the other SWS functions which are using this code. I'll get back with a new test version soon.

Thanks for asking the stretch marker master (I had the same idea :D), and thanks mpl himself for providing code example.

nofishonfriday pushed a commit to nofishonfriday/sws that referenced this issue Feb 19, 2018

nofish
Fix issue reaper-oss#950: Prevent envelopes and stretch markers offse…
…t with BR_SetItemEdges() and "SWS/BR: Trim MIDI item to active content"

nofishonfriday pushed a commit to nofishonfriday/sws that referenced this issue Feb 19, 2018

nofish
Fix issue reaper-oss#950: Prevent envelopes and stretch markers offse…
…t with BR_SetItemEdges() and "SWS/BR: Trim MIDI item to active content"
@nofishonfriday

This comment has been minimized.

Copy link
Collaborator

nofishonfriday commented Feb 19, 2018

New version fore testing:
https://www.dropbox.com/s/qrviacjl553dk6v/reaper_sws64.dll?dl=1

BR_SetItemEdges() now uses Trim... actions (instead of API).
Unintended take env. offsetting with "SWS/BR: Trim MIDI item to active content" should also be fixed.

nofishonfriday pushed a commit to nofishonfriday/sws that referenced this issue Feb 19, 2018

nofish
Fix issue reaper-oss#950: Prevent envelopes and stretch markers offse…
…t with BR_SetItemEdges() and "SWS/BR: Trim MIDI item to active content"
@X-Raym

This comment has been minimized.

Copy link
Contributor Author

X-Raym commented Feb 26, 2018

@nofishonfriday seems to work perfectly !

@nofishonfriday

This comment has been minimized.

Copy link
Collaborator

nofishonfriday commented Feb 26, 2018

Thanks for testing.

nofishonfriday pushed a commit to nofishonfriday/sws that referenced this issue Feb 28, 2018

nofish
Fix issue reaper-oss#950: Prevent envelopes and stretch markers offse…
…t with BR_SetItemEdges() and "SWS/BR: Trim MIDI item to active content"

nofishonfriday pushed a commit to nofishonfriday/sws that referenced this issue Feb 28, 2018

nofish
Fix issue reaper-oss#950: Prevent takes envelopes and stretch markers…
… offset with BR_SetItemEdges() and "SWS/BR: Trim MIDI item to active content"

swstim added a commit that referenced this issue Mar 5, 2018

Merge pull request #955 from nofishonfriday/Fix_Issue_#950_-_prevent_…
…env._+_SM_offset_w._SetItmEdges(),TrimItem()

Fix issue #950: Prevent takes envelopes and stretch markers offset with BR_SetItemEdges() and "SWS/BR: Trim MIDI item to active content"
@X-Raym

This comment has been minimized.

Copy link
Contributor Author

X-Raym commented Apr 25, 2018

@nofishonfriday Hi ! There is a bug in the function, it works on every selected items instead of just the one which is pass as argument.

This is supposed to work on only one item:

item_1 = reaper.GetSelectedMediaItem(0,0)
reaper.BR_SetItemEdges( item_1, 2,4)

But it works on every selected items (if there is an item selection):

Demo

Tested with SWS 2.9.8.

Can you reproduce ?

@nofishonfriday

This comment has been minimized.

Copy link
Collaborator

nofishonfriday commented Apr 25, 2018

Oops, yeah, I see why this happens. Because the now used native actions work on selected items, stupid from me not thinking about this.

So I think it means having to fiddle with stretch markers to properly fix which I suck at (as seen in previous fix attempt). I'll try but if anyone from the other contributers wants to to tackle this I'm not unhappy. :)

Anyway, my current 'fix' needs to be reverted.

@X-Raym

This comment has been minimized.

Copy link
Contributor Author

X-Raym commented Apr 25, 2018

@nofishonfriday you just need to handle unselection items then reselect them within the function :)
It is fast enough to do a good enough solution. And it should be straightfoward to implement, in reascript it is just 4 new lines . :P

@X-Raym

This comment has been minimized.

Copy link
Contributor Author

X-Raym commented Apr 25, 2018

@nofishonfriday Here is my current workarround on script side:

count_sel_items = reaper.CountSelectedMediaItems(0)

-- Save Sel Items
sel_items = {}
for i = 0, cont_sel_items do
	table.insert(sel_items, reaper.GetSelectedMediaItem(0,0))
end

-- Change item length, one after the other
for i, item in ipairs( sel_items ) do
	
	-- Workarround SWS BR_SetItemEdges Bug in v2.9.8
	-- Keep only one item selected
	reaper.SelectAllMediaItems(0, false)
	reaper.SetMediaItemSelected(item.item, true)
		
	reaper.BR_SetItemEdges( item, trim_pos, trim_end)
	
end
	
-- Restore items selection
-- Workarround SWS BR_SetItemEdges Bug in v2.9.8
for i, item in ipairs( sel_items ) do
	reaper.SetMediaItemSelected(item, true)
end

It could be adapted in the API function itself, BR_SetItemEdges being the core of the function.

@nofishonfriday

This comment has been minimized.

Copy link
Collaborator

nofishonfriday commented Apr 25, 2018

Yeah I already thought about doing it this way. :D
I dunno, seems a little dirty somehow, but ok I guess (if no one from the other devs chimes in).

@X-Raym

This comment has been minimized.

Copy link
Contributor Author

X-Raym commented Apr 25, 2018

@nofishonfriday it doesn't look extra efficient, but it has the great advantage of being 'simple to deploy' and backward compatible.
Most of all, it is friendly with all of our concerns (stretch markers etc...).

It is not even said that having to deal with this properly (by making calculation on stretch markers rather than saving items selection) would be that more efficient in performance, so I guess it is fully ok to do this trick 😁

Thanks for your consideration ! 😍

@nofishonfriday

This comment has been minimized.

Copy link
Collaborator

nofishonfriday commented Apr 26, 2018

Well, then... :P (until someone comes up with a better solution, branch is here)
version for testing:
https://www.dropbox.com/s/9vvkc1wuj8cmppa/reaper_sws64_re-fix%20%23950_v01.dll?dl=1

nofishonfriday pushed a commit to nofishonfriday/sws that referenced this issue Apr 26, 2018

nofishonfriday pushed a commit to nofishonfriday/sws that referenced this issue Apr 26, 2018

@X-Raym

This comment has been minimized.

Copy link
Contributor Author

X-Raym commented Apr 26, 2018

@nofishonfriday All good ! I hope it could merge soon cause I have quite some scripts using this API function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment