supernova: include argument names in queryTree #3221

dyfer · 2017-10-03T02:56:05Z

As noted in #3210, scsynth and supernova respond differently to the the queryTree request. This changes the behavior of supernova to match the behavior of scsynth - controls's names are being reported in the reply message.

(
Server.supernova;
// Server.scsynth;
s.waitForBoot({
	SynthDef(\prTest, {arg amp = #[0.1, 0.12], freq = 300, out = 0;
		Out.ar(out, SinOsc.ar(freq, 0, amp))
	}).add;
	s.sync;
	x = Synth(\prTest);
	OSCFunc({arg msg;
		msg.postln;
		x.free;
	}, '/g_queryTree.reply').oneShot;
	s.sendMsg("/g_queryTree", 0, 1);
})
)
//supernova - no patch
// [ /g_queryTree.reply, 1, 0, 1, 1, 1, 1000, -1, prTest, 4, 0, 0.10000000149012, 1, 0.11999999731779, 2, 300, 3, 0 ]

//supernova - with patch
//[ /g_queryTree.reply, 1, 0, 1, 1, 1, 1000, -1, prTest, 4, amp, 0.10000000149012, 1, 0.11999999731779, freq, 300, out, 0 ]

//scsynth
//[ /g_queryTree.reply, 1, 0, 1, 1, 1, 1000, -1, prTest, 4, amp, 0.10000000149012, 1, 0.11999999731779, freq, 300, out, 0 ]

dyfer · 2017-10-03T03:32:20Z

Sorry, this is not ready yet... further tests revealed server crashing with larger messages. I'll keep you posted.

dyfer · 2017-10-03T07:58:22Z

Seems working now.

mossheim · 2017-10-24T00:46:45Z

sorry this has been decaying. i will try to find some time to look at it soon. seems decent at first glance.

mossheim

This works, thanks so much! Logic can be cleaned up though

mossheim · 2018-01-23T01:47:51Z

server/supernova/sc/sc_osc_handler.cpp

+                if(name_of_slot)
+                    p << name_of_slot;
+                else
+                    p << osc::int32(i);


This can be simplified to a single branch:

if (i < scsynth.number_of_slots()) p << scsynth.name_of_slot(i); else p << osc::int32(i);

also style note - put a space after if and not after the open paren

dyfer · 2018-01-26T02:16:31Z

[WIP]
I looked into that and reworked logic, so it's cleaner. It needed few more changes than suggested.
However, I realized another problem: mapped audio controls are not reported properly, so this is WIP right now.

dyfer · 2018-03-03T01:49:48Z

The mappings seem now to be properly reported in response to queryTree for both control and audio buses.

dyfer

I also noticed that when dumping node tree with controls (to the std out, not through OSC - in IDE on mac shortcut cmd-shift-t), mappings are also omitted in supernova. I made a commend next to the line where I think this happens... I'm not sure I'll be able to track this down anytime soon, is it reasonable to open an issue about it separately?

dyfer · 2018-03-03T01:45:13Z

server/supernova/sc/sc_osc_handler.cpp

@@ -1589,7 +1604,7 @@ void dump_controls(rt_string_stream & stream, abstract_synth const & synth, int
        } else
            stream << ", ";

-        stream << synth.get(control_index);
+        stream << synth.get(control_index); /*FIXME: this seems not to check for mapped controls*/


AFAICT this seems to be the place where controls are printed, but omitting the mappings.

dyfer · 2018-04-18T22:27:59Z

I cleaned up commit history, and also added mapped controls when dumping the node tree. I'd appreciate if someone can look this over, thanks!

mossheim

Thanks! Here are my comments

mossheim · 2018-04-19T02:13:15Z

server/supernova/sc/sc_osc_handler.cpp

+                    } else {
+                        bus = (scsynth.mMapControls[i]) - (scsynth.mNode.mWorld->mControlBus);
+                        sprintf(str, "%c%d", 'c', bus);
+                    };


can this not also be a call to getMappedSymbol?

also extra semicolon

mossheim · 2018-04-19T02:13:31Z

server/supernova/sc/sc_osc_handler.cpp

@@ -1575,6 +1588,7 @@ void dump_controls(rt_string_stream & stream, abstract_synth const & synth, int
    const size_t number_of_slots = synth.number_of_slots();

    bool eol_pending = false;
+    char str[10];


declare this closer to use please

mossheim · 2018-04-19T02:14:11Z

server/supernova/sc/sc_synth.cpp

+{
+    bool isMapped = mMapControls[slot_index] != mControls+slot_index;
+    if (isMapped) {
+      int bus;


declare this separately in each branch

also use 4 spaces for indentation, your second level of indentation is 6 instead of 8 I think

mossheim · 2018-04-19T02:14:26Z

server/supernova/sc/sc_synth.cpp

+      } else {
+          bus = mMapControls[slot_index] - mNode.mWorld->mControlBus;
+          sprintf(str, "%c%d", 'c', bus);
+      };


extra semicolon

mossheim · 2018-04-19T02:15:58Z

server/supernova/sc/sc_osc_handler.cpp

+
+                    if (scsynth.mControlRates[i] == 2) {
+                        bus = (scsynth.mMapControls[i]) - (scsynth.mNode.mWorld->mAudioBus);
+                        bus = (int)((float)bus / scsynth.mNode.mWorld->mBufLength);


what is the purpose of these casts? if you are expecting truncation that already happens with integer division in C++.

mossheim · 2018-04-19T02:17:55Z

server/supernova/sc/sc_synth.cpp

+      if (mControlRates[slot_index] == 2) {
+          bus = mMapControls[slot_index] - mNode.mWorld->mAudioBus;
+          bus = (int)((float)bus / mNode.mWorld->mBufLength);
+          sprintf(str, "%c%d", 'a', bus);


clearer to move the character into the format string since it is known at compile time - "a%d", then "c%d" below

mossheim · 2018-04-19T02:19:19Z

server/supernova/server/synth.hpp

@@ -100,6 +100,8 @@ class abstract_synth:

    virtual float get(slot_index_t slot_id) const = 0;

+    virtual bool getMappedSymbol(slot_index_t slot_id, char * str) const = 0;


a brief comment to explain the meaning of the return value would be nice

mossheim · 2018-04-19T05:01:06Z

server/supernova/server/synth.hpp

@@ -100,6 +100,8 @@ class abstract_synth:

    virtual float get(slot_index_t slot_id) const = 0;

+    virtual bool getMappedSymbol(slot_index_t slot_id, char * str) const = 0; //returns true (and writes characters to *str) if the control at slot_id is indeed mapped, otherwise returns false


could you please use a javadoc comment (/** ... */ or ///) on the line above? this matches the documentation style of supernova and also keeps text at a reasonable width

mossheim · 2018-04-19T05:03:05Z

server/supernova/sc/sc_synth.cpp

+            sprintf(str, "a%d", bus);
+        } else {
+            int bus;
+            bus = mMapControls[slot_index] - mNode.mWorld->mControlBus;


this is fine, for future reference you can declare while initializing (int bus = ...), this will save space and reduce visual noise

you probably know that already, just stating my preference :)

dyfer · 2018-04-19T05:04:53Z

Thank you Brian for the feedback! I made the requested corrections. I can also ~~squish~~ squash the last (fixing) commit into the previous one if desired.

mossheim · 2018-04-19T05:15:25Z

Thanks for being so quick to respond @dyfer. I think the whole thing can be squashed on merge, it's a small enough PR in terms of LoC

dyfer · 2018-06-05T00:03:40Z

Hey @brianlheim, does this PR need more changes?

mossheim

Looks good, thanks for your patience and I apologize for how long it took to review this.

Cherry-pick #3221 into 3.10

mossheim added the comp: supernova label Oct 24, 2017

patrickdupuis added this to the 3.10 milestone Jan 17, 2018

mossheim suggested changes Jan 23, 2018

View reviewed changes

mossheim added the Work In Progress (WIP) - don't merge yet label Jan 26, 2018

dyfer commented Mar 3, 2018

View reviewed changes

dyfer added 3 commits April 18, 2018 15:21

supernova: include argument names on /g_queryTree message

25fd0bf

supernova: report mapped controls on /g_queryTree message

a9805f4

supernova: report mapped controls on /g_dumpTree message

077d79a

dyfer force-pushed the topic/supernova-query-control-names branch from 9b113ad to 077d79a Compare April 18, 2018 22:25

mossheim suggested changes Apr 19, 2018

View reviewed changes

mossheim reviewed Apr 19, 2018

View reviewed changes

supernova: report mapped controls - syntax fixes

e21e35a

dyfer force-pushed the topic/supernova-query-control-names branch from a3e4b20 to e21e35a Compare April 19, 2018 05:16

dyfer mentioned this pull request May 31, 2018

/g_queryTree response: control name vs index - scsynth vs supernova #3210

Closed

patrickdupuis removed the Work In Progress (WIP) - don't merge yet label Aug 31, 2018

mossheim self-requested a review September 16, 2018 00:38

mossheim approved these changes Sep 18, 2018

View reviewed changes

mossheim merged commit e6f5086 into supercollider:develop Sep 18, 2018

patrickdupuis mentioned this pull request Sep 19, 2018

Cherry-pick #3221 into 3.10 #4071

Merged

mossheim added a commit that referenced this pull request Sep 23, 2018

Merge pull request #4071 from patrickdupuis/topic/cp3221

7a6843c

Cherry-pick #3221 into 3.10

joshpar mentioned this pull request Jul 16, 2019

Fix build with boost-1.70 #4491

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

supernova: include argument names in queryTree #3221

supernova: include argument names in queryTree #3221

dyfer commented Oct 3, 2017

dyfer commented Oct 3, 2017

dyfer commented Oct 3, 2017

mossheim commented Oct 24, 2017

mossheim left a comment

mossheim Jan 23, 2018

dyfer commented Jan 26, 2018 •

edited

Loading

dyfer commented Mar 3, 2018 •

edited

Loading

dyfer left a comment •

edited

Loading

dyfer Mar 3, 2018

dyfer commented Apr 18, 2018 •

edited

Loading

mossheim left a comment

mossheim Apr 19, 2018

mossheim Apr 19, 2018

mossheim Apr 19, 2018

mossheim Apr 19, 2018

mossheim Apr 19, 2018

mossheim Apr 19, 2018

mossheim Apr 19, 2018

mossheim Apr 19, 2018

mossheim Apr 19, 2018

mossheim Apr 19, 2018

mossheim Apr 19, 2018

mossheim Apr 19, 2018

dyfer commented Apr 19, 2018 •

edited

Loading

mossheim commented Apr 19, 2018

dyfer commented Jun 5, 2018

mossheim left a comment

		@@ -100,6 +100,8 @@ class abstract_synth:

		virtual float get(slot_index_t slot_id) const = 0;

		virtual bool getMappedSymbol(slot_index_t slot_id, char * str) const = 0;

supernova: include argument names in queryTree #3221

supernova: include argument names in queryTree #3221

Conversation

dyfer commented Oct 3, 2017

dyfer commented Oct 3, 2017

dyfer commented Oct 3, 2017

mossheim commented Oct 24, 2017

mossheim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dyfer commented Jan 26, 2018 • edited Loading

dyfer commented Mar 3, 2018 • edited Loading

dyfer left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dyfer commented Apr 18, 2018 • edited Loading

mossheim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dyfer commented Apr 19, 2018 • edited Loading

mossheim commented Apr 19, 2018

dyfer commented Jun 5, 2018

mossheim left a comment

Choose a reason for hiding this comment

dyfer commented Jan 26, 2018 •

edited

Loading

dyfer commented Mar 3, 2018 •

edited

Loading

dyfer left a comment •

edited

Loading

dyfer commented Apr 18, 2018 •

edited

Loading

dyfer commented Apr 19, 2018 •

edited

Loading