CNMAT / CNMAT-odot

Multi-paradigm Dynamic Programming
Other
115 stars 11 forks source link

move o.atomize to main release? #329

Closed ramagottfried closed 5 years ago

ramagottfried commented 8 years ago

I am regularly using o.atomize and have had no issues with it for years -- any chance we could move that out of the dev folder so students don't have to manually add the dev folder to the Max search path?

maccallum commented 8 years ago

It's fine with me. There was some hesitation around the 1.0 release regarding the design of the object, but at this point, I don't see us making any significant changes to it or revisiting the question of how to cross the odot/Max boundary.

Comments?

adrianfreed commented 8 years ago

This is a tough one. Without a plan for who will maintain (and improve) odot in max in the future there is a danger we are bringing a headache into the future for somewhat as yet unknown.

On Aug 31, 2016, at 20:25, John MacCallum notifications@github.com wrote:

It's fine with me. There was some hesitation around the 1.0 release regarding the design of the object, but at this point, I don't see us making any significant changes to it or revisiting the question of how to cross the odot/Max boundary.

Comments?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

ramagottfried commented 8 years ago

Am I correct that the main issue with o.atomize is that it doesn't output sub-bundles but only an address followed by a FullPacket message?

If so, what's the difference between that and something silly like sending a bogus pointer into an o.route object? For instance here's an easy crash:


----------begin_max5_patcher----------
324.3ociRsrSCCCD7ryWgkOGB4QoE3CfybGgPNIKst3XGY6TRUU+2wYcBjR4
QuXqc1Ymc2w9PDgUp6AKidO8IJgbHhPPnA.xXLg0v6qjbKRi0.VKeMvhC4bP
uCwenSJejW8F3nYz7oz5NmDbt8sPnGLF84wTV2dIhxlHq5ZDJOcrQ4ifsbW0
FgZ8KFnxEDoHeYRZLMaAdc6pgy77jzOkVTi5pK2dUwLwCCCpd1.3wnngi3Kb
yUv6dEOaw0IFuv.85W05eesim6PWrIj8+lvcE3UZ5e4BY+rKjOyEvxXRg56e
GvdOfep0X0clpo0azooe07Zv5DJtSnUy3jcBmMh5Z.SmNUkvxKk.N3omMb71
1cfwNJINH9WnsZyP3pXLTnBg32GlA1Il3eChvMdiz4cwNSv26WtfEJUWCFUm
X76guyGi9.PfHoan
-----------end_max5_patcher-----------
ramagottfried commented 8 years ago

of course you could do the same thing with map/lambda/getaddresses, but it's about 10x slower:


----------begin_max5_patcher----------
1611.3oc0YssaaiCD8Y6uBA8TxV2DQcyxKvhhEXw91BruGGXPIQ6vVIRAIpT
2sn+6KuHoHISYS6l31BDnXM71YNyvYFQ904yrio6QU1V+t0CVyl804ylIEID
Lq48Y14v8IYvJY2ryQUUvcH6Ep1Xn8Lobm6bAg9KiZaHCSPIzZhrU2FgEPVx
SXxtMknDlZU87ityYgk2ROw+BbjuDbmi0iMigTmSqYYHlb8AMRwoxUkF+w26
F1tlEknJDgAYXJo+RzLqpkvo4Q2Bnlc1WJPptaa2eswj1ktUIpXeIS1Uaagf
uMet3whua9ywy024r4u.eoV46b47WvI3uPOW4rFE7SL+ceUcr0eWmk8uvjOg
XVdgV.emkd9K8VE334E5qkYCmlYCCkTp6pnWX1nUmEy5eBlcYfjYcib9IfYI
nOyA8ADKyJ1Jy9HNfdR9ArzW9OGoV35NIO4pgm.mxCzcobxCBMimhgjc1Klj
u.uk7ULhj7jEGRmly7.QlwY57sbMiy7bLz2ZaFEx9Q3foHLL4X70Jfjg.qtb
eLuSwWpMgtN.C2Kx8uxvUreD9Xz6fLZN9+PFvYpPLMwxNKWLPWxnCT9WBz9i
P8Krxf4wv2eZR.rLrGIrZ0EPBKsOynwZT89.DU1nvMZ7L6s3LzynxJt+XuwO
yFVTzS7rdCQPSejJmnkK5DgIJQtchJQOiaGePmTXImlXbNptTgu8cIGESCME
URpwRnnDxMXyamvtTW.UH4HoWVnubCieTusLbi6tLJ2GQRlNsBoEHBlzeS3f
lSQag0YrMaoDVkv3Jfgv5oo8svDzjCl.yUZ2eVhgsIvlYuqDmRIBPLfqEhaW
NtaSfJcSekQ1CBrPyf49IbdYhFq3JYcULrTXJhUdDsVHaFklMrotwkg1xZZt
.SHiXQFsX5FKw6d5HiMlxaL+XysrkpM0DUqa3aHYapfOOjsYvrrlcnCm98PB
NGxPLrxD35z0Hh.4J5SUIkzrrA5qpkm0zRJ2KNA8YbJ6I4B02Yf2cbQqSjcm
UNEuCUwFJiA2UMTxA6P4hpia1ktggxKx3ZgrC+kxyx5ef6sVZ2SUe4qm5u+r
efsAxOV.tww3K4QbPV2mVmWb+u0uO5h1qRdMYAESmjbb.uv9qj1rdZB9OYTP
cIAl0lHnI1xExcz6P6KJuKgGuRL.cbXNr3l0DKY5hT3MvEhWrrtWV9u0G9ve
v8ZUh3SJdG4l01J5ds86tQ0q28taWX8.bg0yvrZzMvae7VwH3B2gXvzTdbrJ
T0Mbg21GB59HCUvdMwsNpMswX5E.5dyPi5DgPGYu8FybIzLUdjGrFT5HewGs
zGqr.i8SdE8HjypQaTToq75ddDFEnm1.AGeexzZri9cF8jlPyyQD1q+NFIJM
hfZ9rAiXnIbr.ChjXRvgyfBdIurXm1DwfkZmnc8bSEstLo0f0rUvZnhxSvvv
jtpTdnKD4n98DNMEQFSFo3JQNMU1UirimKhEdgFAYueZfbngHF7ZwxVONueQ
r1h5XR2npYaCjwJww77rUiqJ+rpsfWnaLLqoxgtJOOVgFuTMx7Wv6a3Yg4DF
F3FFEAh7C7cN2yBCzTeQzEeVX.e6ekNUqlikFDFdwm3.vw9W0ymp8P4ANW74
Ssx9WwCZx2o4fl.WrYe5CM4pbhQTdAw4EzJMQE1RoxkYM4d9m2IQO+mULzyH
9KdR4zX9O8kxqE+7qqIqYbwhQt19Knp01qIeSa3ilShPWEt58wTUhoN131Zc
COoOl9pZ6aA76q5CplEDEsXz+.eOm2tLSRbMIMCsICQ1o9.UPf1NjBYv1KjZ
g0JwZCVJdn7zDODxb.brYvefPN4IFuiqXR.MO52EeeQyAGNTMSi.Oqjvw2rI
wUGV.AhGgMpA.LTglZp7l.ORER7CSvSeXyM.8wyR8yhe3gHIbwqq0oSMzab.
c5AvEzwZAxBe5s4W5FNrRWUrfwEe0DQ3fhtV0uVpIJ3RbOpVftNcPwVGVn0A
glLEN9l.GvfN8VBGwcobZ7Db0nGwAueZ73e0viqmA3YDneywC3T7i60ie.lf
GuqG+.Lw+w8pgGSbmE2yxUBNiVpIvy0a6k3CENIdVc8nGGC7luda1MIWwHF7
s1WFXfs5RbcTo4GcseBfL559FcUeGdMeSeEeiudO9J+s4+OfZWZlj
-----------end_max5_patcher-----------
adrianfreed commented 8 years ago

my feeling is that we shouldn’t generate anything that will generate a crash later on.

On Aug 31, 2016, at 23:41, rama gottfried notifications@github.com wrote:

Am I correct that the main issue with o.atomize is that it doesn't output sub-bundles but only an address followed by a FullPacket message?

If so, what's the difference between that and something silly like sending a bogus pointer into an o.route object? For instance here's an easy crash:

----------begin_max5_patcher---------- 324.3ociRsrSCCCD7ryWgkOGB4QoE3CfybGgPNIKst3XGY6TRUU+2wYcBjR4 QuXqc1Ymc2w9PDgUp6AKidO8IJgbHhPPnA.xXLg0v6qjbKRi0.VKeMvhC4bP uCwenSJejW8F3nYz7oz5NmDbt8sPnGLF84wTV2dIhxlHq5ZDJOcrQ4ifsbW0 FgZ8KFnxEDoHeYRZLMaAdc6pgy77jzOkVTi5pK2dUwLwCCCpd1.3wnngi3Kb yUv6dEOaw0IFuv.85W05eesim6PWrIj8+lvcE3UZ5e4BY+rKjOyEvxXRg56e GvdOfep0X0clpo0azooe07Zv5DJtSnUy3jcBmMh5Z.SmNUkvxKk.N3omMb71 1cfwNJINH9WnsZyP3pXLTnBg32GlA1Il3eChvMdiz4cwNSv26WtfEJUWCFUm X76guyGi9.PfHoan -----------end_max5_patcher-----------

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

maccallum commented 8 years ago

I whole heartedly agree, but o.route has the same disease. I'd say keep it in the dev folder though---there are a lot of implicit design decisions that were based on prior work and weren't well-thought out in the current context.

ramagottfried commented 8 years ago

I agree that we shouldn't generate anything that could crash.

Maybe a more robust solution would be to never send out the length and pointer numbers from o.route/atomize but only the "FullPacket" symbol indicating that there was a bundle there. That way no odot object will attempt to access a stale pointer, saving everyone headaches in the future.

ramagottfried commented 8 years ago

just to clarify, of course I mean just in the case of sub-bundles, so atomized:

/foo : { /bar : 1 } becomes: /foo FullPacket

instead of the current: /foo FullPacket 1 12346578

maccallum commented 8 years ago

I think I prefer dangerous but functional over safe but crippled…

On Aug 31, 2016, at 4:27 PM, rama gottfried notifications@github.com wrote:

just to clarify, of course I mean just in the case of sub-bundles, so atomized:

/foo : { /bar : 1 } becomes: /foo FullPacket

instead of the current: /foo FullPacket 1 12346578

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/CNMAT/CNMAT-odot/issues/329#issuecomment-243933000, or mute the thread https://github.com/notifications/unsubscribe-auth/ABjjdX36p3Q4CqmlG_8aG4adKIHAeam5ks5qlg3WgaJpZM4Jx4ME.

adrianfreed commented 8 years ago

is unsafeFullPacket 1 12345678 a compromise?

On Sep 1, 2016, at 01:27, rama gottfried notifications@github.com wrote:

just to clarify, of course I mean just in the case of sub-bundles, so atomized:

/foo : { /bar : 1 } becomes: /foo FullPacket

instead of the current: /foo FullPacket 1 12346578

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

ramagottfried commented 7 years ago

@maccallum do you have any new thoughts about this? Should we make it unsafeFullPacket?

The o.expr.codebox help patch uses o.atomize to populate the function menus, and it's used in many other patches as well, so I'd like to move o.atomize out of the dev folder if possible.

ramagottfried commented 7 years ago

I just made the change in libomax to give it a try -- unsafeFullPacket seems pretty smart to me: it's unsupported by odot objects, but "experts" could potentially still find the memory there (if they're lucky).

ramagottfried commented 7 years ago

... I'm having second thoughts now. Changing this in omax_util_oscMsg2MaxAtoms also effects routing subbundles with o.route, which is an important patching paradigm. So, if we think that the bundle max atom FullPacket is used differently when output by o.atomize than o.route (which probably is correct), then maybe the unsafeFullPacket could be used only in o.atomize.

Or, we could do some nominal validity check instead of crashing -- a few if() functions shouldn't slow things down too much, right?

@maccallum any thoughts on this?

maccallum commented 7 years ago

Yeah, changing to unsafeFullPacket doesn’t really help that much, and it’s a big job. There is already a safety check in place, but it can never be very good. Once the memory is freed, there’s no guarantee of what will be put in there—you might get a new packet of a length that doesn’t correspond to the length you’re expecting.

Unfortunately this is not a straightforward fix, otherwise it’d be done by now.

Can we automagically add /dev to one’s search path as part of an installation process?

On Oct 16, 2017, at 3:57 PM, rama gottfried notifications@github.com wrote:

... I'm having second thoughts now. Changing this in omax_util_oscMsg2MaxAtoms also effects routing subbundles with o.route, which is an important patching paradigm. So, if we think that the bundle max atom FullPacket is used differently when output by o.atomize than o.route (which probably is correct), then maybe the unsafeFullPacket could be used only in o.atomize.

Or, we could do some nominal validity check instead of crashing -- a few if() functions shouldn't slow things down too much, right?

@maccallum https://github.com/maccallum any thoughts on this?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/CNMAT/CNMAT-odot/issues/329#issuecomment-336894777, or mute the thread https://github.com/notifications/unsubscribe-auth/ABjjdYgNoFXbJbem8BsB0NV8spYB9wwYks5ss2CugaJpZM4Jx4ME.

ramagottfried commented 7 years ago

ok -- as for adding to the search path for the release package, basically our only solution that I can see that makes sense would be to add a /dev subfolder in the main /externals folder.

if there are some that we really don't want being used without manually adding them then we could have another folder for those, what do you think?

maccallum commented 7 years ago

We could move dev into externals, but if we include these automatically, I think they should post a warning about them being subject to change.

What are the patches that use them? Can they be taken out or replaced?

On Oct 16, 2017, at 4:49 PM, rama gottfried notifications@github.com wrote:

ok -- as for adding to the search path for the release package, basically our only solution that I can see that makes sense would be to add a /dev subfolder in the main /externals folder.

if there are some that we really don't want being used without manually adding them then we could have another folder for those, what do you think?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/CNMAT/CNMAT-odot/issues/329#issuecomment-336911000, or mute the thread https://github.com/notifications/unsubscribe-auth/ABjjdSMtMYbcgtAs8fl0EUanUgYukvzoks5ss20NgaJpZM4Jx4ME.

ramagottfried commented 7 years ago

for patches in the release that use o.atomize, so far the o.expr.codebox function menu generator patch is the main one.

I guess probably most of my patches that use o.atomize are using it to simplify message sending to non-odot objects, like: /spat/source/1/xyz : [0,1,2] -- I o.route the /spat and then o.atomize it, split off the /, and patch directly to spat.

just for argument's sake, o.route has the same issue, so why warn about o.atomize only?

ramagottfried commented 7 years ago

the o.expr.codebox help patch's use of o.atomize in two different ways is a good example of why it's worth keeping and adding to the main release folder -- in one case its used to iterate and capture only the address to store in a coll, in the second case it allows you to slice off the address and only keep the value.

in the first case it's easy to replace with getaddresses() -> o.route -> iter, but in the second case it's slightly more complicated: you'd need to either use o.messageiterate (which is also in the dev folder, and maybe the best choice), or you could first get all the addresses, and then rename the values something like /out/0, /out/1, /out/2, ... and then do o.route /out/* which would iterate the values only.

In regards to the worries about stale FullPacket pointers, I'm not sure how much safer any of the odot objects are really. They all output a pointer which could go stale if not used immediately. If we want to address this concern, I'm thinking that maybe the best thing to do is to make a well marked warning patch which explains the issue. It's important that anyone learning odot needs to know about how to deal with this. There is already a mention I believe in the o.var help patch, but it could be better flagged in the overview or something like that.

ramagottfried commented 7 years ago

working on an o.safety.readme patch, and just noticed that o.atomize actually does a good thing when it outputs the atomized iteration -- it waits until it's finished outputting everything before freeing the memory.

void oroute_atomizeBundle(void *outlet, long len, char *bndl)
{
    t_osc_bndl_it_s *it = osc_bndl_it_s_get(len, bndl);
    while(osc_bndl_it_s_hasNext(it)){
        t_osc_msg_s *msg = osc_bndl_it_s_next(it);
        int natoms = omax_util_getNumAtomsInOSCMsg(msg);
        t_atom atoms[natoms];
#ifdef OMAX_PD_VERSION
        if(omax_util_oscMsg2MaxAtoms(msg, atoms))
                {
                     object_error((t_object *)x, "pure data does not like { }, hopefully someone will fix this eventually\n");
                    return;
                }
#else
        omax_util_oscMsg2MaxAtoms(msg, atoms);
#endif
        t_symbol *address = atom_getsym(atoms);
        outlet_anything(outlet, address, natoms - 1, atoms + 1);
    }
    osc_bndl_it_s_destroy(it);
}
maccallum commented 7 years ago

On Nov 24, 2017, at 11:09 AM, rama gottfried notifications@github.com wrote:

the o.expr.codebox help patch's use of o.atomize in two different ways is a good example of why it's worth keeping and adding to the main release folder —

There’s no doubt that it’s a useful object, and the dev folder doesn’t mean that it’s going to be thrown away or something. The only difference between the main folder and dev is that everything in the main folder is guaranteed to have a stable API, while things in dev are not. I think the likelihood that o.atomize would change at some point in the future is way to high to move it into the main folder.

Is there really no way of adding the dev folder to the Max search path automatically?

in one case its used to iterate and capture only the address to store in a coll, in the second case it allows you to slice off the address and only keep the value.

in the first case it's easy to replace with getaddresses() -> o.route -> iter, but in the second case it's slightly more complicated: you'd need to either use o.messageiterate (which is also in the dev folder, and maybe the best choice), or you could first get all the addresses, and then rename the values something like /out/0, /out/1, /out/2, ... and then do o.route /out/* which would iterate the values only.

In regards to the worries about stale FullPacket pointers, I'm not sure how much safer any of the odot objects are really. They all output a pointer which could go stale if not used immediately. If we want to address this concern, I'm thinking that maybe the best thing to do is to make a well marked warning patch which explains the issue. It's important that anyone learning odot needs to know about how to deal with this. There is already a mention I believe in the o.var help patch, but it could be better flagged in the overview or something like that.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/CNMAT/CNMAT-odot/issues/329#issuecomment-346791694, or mute the thread https://github.com/notifications/unsubscribe-auth/ABjjdQhpIjmFnqxq6PmBRszAzCOBaO7oks5s5pXUgaJpZM4Jx4ME.

ramagottfried commented 7 years ago

ok we can keep in /dev if you really think that's the best place for it.

after researching on the forum so far I think unfortunately there is not a documented way to automatically add a folder to the search path from the package, before loading a patch (e.g. from an init folder script, maybe it's possible but there is no documentation that I can see).

you can use the filepath object to temporarily add a path from a patch. so potentially for the o.expr help patch we could add the dev folder to the search path and then load the function umenu scripting patch after adding the folder to the search path.

but actually, if we're adding it automatically to the search path, it seems like we might as well make the /dev folder a sub-folder in the main /externals folder, right?

maccallum commented 7 years ago

Yeah, actually, that might not have been a good suggestion on my part. The idea was to make people add the dev folder manually so that they know what they’re getting themselves into.

How necessary is o.atomize in the o.expr.codebox help file? Is there a workaround that would allow us to take it out?

On Nov 27, 2017, at 4:38 PM, rama gottfried notifications@github.com wrote:

ok we can keep in /dev if you really think that's the best place for it.

after researching on the forum so far I think unfortunately there is not a documented way to automatically add a folder to the search path from the package, before loading a patch (e.g. from an init folder script, maybe it's possible but there is no documentation that I can see).

you can use the filepath object to temporarily add a path from a patch. so potentially for the o.expr help patch we could add the dev folder to the search path and then load the function umenu scripting patch after adding the folder to the search path.

but actually, if we're adding it automatically to the search path, it seems like we might as well make the /dev folder a sub-folder in the main /externals folder, right?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/CNMAT/CNMAT-odot/issues/329#issuecomment-347220314, or mute the thread https://github.com/notifications/unsubscribe-auth/ABjjdYzmbthnF-cxBDGvBUmqF1Dd6-64ks5s6tdygaJpZM4Jx4ME.

ramagottfried commented 7 years ago

it's not a not "simple" workaround but it's possible, not that big of a deal.

I've taken o.atomize out of almost all of the m158 materials, so that should be ok also.

but... I'm pretty sure the whole of odot has that potential issue of crashing if a user sends a garbage pointer, so I'm not sure if o.atomize in particular is in need of development on that aspect more than other objects. but I understand if you think o.atomize might change for another reason, then for sure it makes sense to keep it separated.

p.s. that would be cool if we could avoid crashing when getting a garbage pointer, but I'm not sure how to do it -- the OSC_MEM_VALIDATE macro is the thing that crashes, when it dereferences attempting to test the first character

#define OSC_MEM_VALIDATE(ptr) ((ptr) ? !(*ptr == '#' || *ptr == '/') : 1)
ramagottfried commented 7 years ago

p.p.s. another way we might want to consider for the max objects is avoid the issue by doing something like cycling does with dictionaries and have certain objects maintain a bundle with a unique name that always has a valid state ... not sure what side effects that might cause

equilet commented 7 years ago

Is there really no way of adding the dev folder to the Max search path automatically?

There is a filepath object for this purpose

ramagottfried commented 7 years ago

hi @equilet -- yeah, I mentioned the filepath object approach above also, but maybe I missed something? my understanding is that it adds the file with a set message, which needs to happen before loading the patch right? so then we'd have to have a filepath object that first adds the /dev folder temporarily and then loads the patch with o.atomize to generate the umenus afterwards which seems a little sketchy maybe. that'd be cool if you could give it a folder location as an argument to load before loading the rest of the patch, but I think it's just for setting the path type... but let me know if I missed something!

otherwise, it's a bit kludgy, but not that hard to replicate the o.atomize functionality with o.expr and iterate with route /* , so I guess that's probably the way to go.

ramagottfried commented 7 years ago

ok, it's not that bad. thank you wildcard iteration:

/addrs = getaddresses(),
map( 
  lambda([i,a],
    assign("/out/"+i, [a, value(a)])
  ), aseq(1, length(/addrs)), /addrs
)
---> o.route /out/* --->
equilet commented 5 years ago

@ramagottfried can this be closed?