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

Inconsistent @startframe behavior in fluid.bufstats~ #231

Open
rconstanzo opened this issue Jul 3, 2023 · 8 comments
Open

Inconsistent @startframe behavior in fluid.bufstats~ #231

rconstanzo opened this issue Jul 3, 2023 · 8 comments
Labels
good first issue Good for newcomers

Comments

@rconstanzo
Copy link

As outlined in this thread on the discourse it seems that @startframe (and presumably @numframes) is only being applied to the @source buffer and not the @weights buffer, leading to errors being thrown even when both buffers satisfy all the conditions of fluid.bufstats~:

a single-channel
exactly the same amount of frames as source
all values must be positive (anything lower than 0 will be rejected)

This creates a situation where the @weights buffer needs to be manually trimmed based on the @startframe / @numframes attributes of fluid.bufstats~.

Expected behavior:
@startframe / @numframes truncating is applied to both @source and @weights buffers (as well as the right inlet (the "weights inlet").

@rconstanzo
Copy link
Author

rconstanzo commented Jul 3, 2023

Also, I posted this in the flucoma-max repo as I can confirm that is the case there, but I imagine this happens across all CCL/contexts.

@jamesb93 jamesb93 transferred this issue from flucoma/flucoma-max Jul 4, 2023
@jamesb93
Copy link
Member

jamesb93 commented Jul 4, 2023

I think it could be a case of just trimming the weights vector before it is supplied to the MultiStats processor... I could possibly take a look but my C++ fu is not good.

@jamesb93
Copy link
Member

jamesb93 commented Jul 4, 2023

Do you have a patch to test with @rconstanzo?

@rconstanzo
Copy link
Author

(why do I never get notifications from github responses...)

Here you go:
Screenshot 2023-07-05 at 1 15 49 PM

starrtframe error.maxpat.zip

@weefuzzy weefuzzy added the good first issue Good for newcomers label Aug 11, 2023
@weefuzzy
Copy link
Member

Agreed. source and weights should be treated consistently, rather than wanting weights to be already the effective size of source.

However, this is a textbook example of a hidden interface break (i.e. someone, somewhere will be relying on old behaviour).

So, with heavy heart, great regret etc, I think that to fix this we need to make it optional with a new param 😢 (trimWeights or something). Sorry @rconstanzo – I agree it should have been the default all along.

@rconstanzo
Copy link
Author

Hmm, perhaps I'm overlooking something.I don't see how this would break anything.

All that means is the person would have been manually changing the weights size before sending it into fluid.bufstats~ anyways, which would still work with a fixed interface.

Presently:

  • create a source buffer
  • create a weights buffer
  • crop/copy weights buffer to same size as source
  • send both buffers

Proposed:

  • create a source buffer
  • create a weights buffer
  • (crop/copy weights buffer to same size as source)
  • send both buffers

It would still work if you manually crop the buffer ahead of time (I don't see why fluid.bufstats~ would care that you manually trimmed your buffer or not ahead of time).

@weefuzzy
Copy link
Member

Any code that assumes that startFrame doesn't apply to the weights buffer, and uses a changing startFrame for the source buffer (perhaps because the weights are generated in a different way to you) would break if startFrame suddenly started applying to the weights buffer as well.

@rconstanzo
Copy link
Author

rconstanzo commented Aug 11, 2023

Ah fair, yeah that would indeed break.

My suspicion is that (somehow) no one has used @startframe with fluid.bufstats~ for weighted buffers as it would have popped up as a bug before, but best not risk it.

Plus this has the unintended side effect of immediately throwing an error (trimeweights is not a valid attribute) if people run the patch on an older version of FluCoMa, rather than it silently failing or throwing a (confusing) error later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants