Skip to content

Commit

Permalink
outName changed from atom to symbol
Browse files Browse the repository at this point in the history
  • Loading branch information
@fearn-e
fearn-e committed Aug 3, 2023
1 parent 5b841d3 commit d410b7e
Showing 1 changed file with 8 additions and 6 deletions.
14 changes: 8 additions & 6 deletions source/include/clients/nrt/FluidListToBuf.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ struct FluidListToBuf
t_object* defaultOut;
t_buffer_ref* outputRef;
t_symbol* defaultOutName{nullptr};
t_atom outName;
t_symbol* outName;
index axis{0};
index canResize;
index startChannel{0};
Expand Down Expand Up @@ -85,7 +85,7 @@ void main()
class_addmethod(FluidListToBufClass, (method) FluidListToBuf_dblclick,
"dblclick", A_CANT, 0);

CLASS_ATTR_ATOM(FluidListToBufClass, "destination", 0, FluidListToBuf, outName);
CLASS_ATTR_SYM(FluidListToBufClass, "destination", 0, FluidListToBuf, outName);
CLASS_ATTR_LABEL(FluidListToBufClass, "destination", 0, "Output Buffer");
CLASS_ATTR_ACCESSORS(FluidListToBufClass, "destination", FluidListToBuf_getOut,
FluidListToBuf_setOut);
Expand Down Expand Up @@ -130,7 +130,7 @@ void* FluidListToBuf_new(t_symbol*, long argc, t_atom* argv)
&bufferArgs);

x->output.reset(new MaxBufferAdaptor((t_object*) x, x->defaultOutName));
atom_setsym(&x->outName, x->defaultOutName);
x->outName = x->defaultOutName;
{
auto buf = MaxBufferAdaptor::Access(x->output.get());
buf.resize(argCount > 0 ? atom_getlong(argv) : 0,
Expand All @@ -150,12 +150,12 @@ t_max_err FluidListToBuf_setOut(FluidListToBuf* x, t_object* /*attr*/,
t_symbol* s = atom_getsym(argv);
if (s == gensym(""))
{
atom_setsym(&x->outName, x->defaultOutName);
x->outName = x->defaultOutName;
x->output.reset(new MaxBufferAdaptor((t_object*) x, x->defaultOutName));
}
else
{
atom_setsym(&x->outName, s);
x->outName = s;
x->output.reset(new MaxBufferAdaptor((t_object*) x, s));
}
}
Expand Down Expand Up @@ -254,7 +254,9 @@ void FluidListToBuf_list(FluidListToBuf* x, t_symbol* /*s*/, long argc,
std::transform(argv, argv + count, frames.begin(),
[](const atom& a) -> float { return atom_getfloat(&a); });

outlet_anything(x->outlet, bufferSym, 1, &x->outName);
t_atom outNameAtom;
atom_setsym(&outNameAtom, x->outName);
outlet_anything(x->outlet, bufferSym, 1, &outNameAtom);
}
}

Expand Down

5 comments on commit d410b7e

@fearn-e
Copy link
Owner Author

@fearn-e fearn-e commented on d410b7e Aug 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks to me like outName should only ever be a symbol and a better fix is to change it to be a t_symbol*. The only place it is really used as an atom is for the call to outlet_anything() but for that one could make a local t_atom and set it to the symbol, which would be a better solution.

@AlexHarker I've attempted to implement your suggestions, please let me know if you have anything further as I'm still not very familiar with Max's t_atom and t_symbol types.

Further, line 34 t_symbol* defaultOutName{nullptr}; has an array with nullptr inside it for some reason. When setting outName on line 35, both t_symbol* outName; and t_symbol* outName{nullptr}; seem to produce an object in Max that works and has a buffer name one can set without crashing. Would you know what difference (if any) this makes?

@AlexHarker
Copy link

@AlexHarker AlexHarker commented on d410b7e Aug 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like what I would have done.

t_symbol * is generally a pointer to a single t_symbol, rather than an array (Max uses a system where strings are cached into unique pointers that can be compared directly because in the old days it would have made things much faster). Setting it using {} is just a different kind of C++ initialiser syntax.

I would also set outName to a nullptr (as you suggest above) for consistency, although technically I don't think this struct ever gets constructed in a C++ sense. That could do with addressing and checking elsewhere. @weefuzzy might be able to check my thinking there.

@weefuzzy
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking at this. Just so I'm all caught up: inferring back through flucoma-max issues, bumping the SDK version on windows revealed (among other bad things) that this objects is using the wrong attribute macro for its type? (atom vs symbol). The fix being pursued here is just to change the actual type to symbol anyway because it seems only to need atom-ness in one place?

Seems reasonable, but if it's just an issue of fixing flucoma#371 in main then my preference would be for the minimal change sufficient to fix the bug. If that is just correcting the macro, then go with that. The better-ness of using symbol here can then be explored in a different PR. OTOH, if correcting the macro isn't sufficient, then doing it this way seems more justifiable.

(I honestly can't remember if there was any better reason I had for making this an atom in the first place. I'd need to take a proper look at the code again (which is unlikely to happen in the next few days))

@AlexHarker
Copy link

@AlexHarker AlexHarker commented on d410b7e Aug 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @weefuzzy - thanks - I am sure that it is better for it to be a symbol, because it is only ever set to a symbol, (in fact I think it is set directly from a variable that can only be a symbol) so I believe the changes as proposed to be totally sane. Likely the atom before was not set correctly on object load.

What I was looking for your input on was the issue of setting items in the constructor using initializer brackets {nullptr} as (as far as I can see) the struct is actually not constructed (as it is a max object it is made with object_alloc, which may zero, but won't respect constructor values). Can you check my thinking on that?

I guess I'm also a little hazy on why this is a attribute (or at least if it should be read-only) as it seems to get set from other places than I'd expect and I can't obviously see a place where the attribute value would be used, but I've only skimmed it.

Last thing is that @junofern is one of the CCL interns, so we will be seeing more PRs shortly...

@fearn-e
Copy link
Owner Author

@fearn-e fearn-e commented on d410b7e Aug 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AlexHarker just making a note here, I have 2 branches on this repo now, the one associated with the original PR is now reverted to just changing the CLASS_ATR, and there is a separate one that has all these symbol changes in it.

Please sign in to comment.