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

Add the argument bounds to the .scope and .plotTree methods to define their bounds; add the argument position to the .meter method to define its position #6230

Closed
wants to merge 24 commits into from

Conversation

prko
Copy link
Contributor

@prko prko commented Mar 6, 2024

Purpose and Motivation

.scope has no argument to define its window location. Often it is good to see this window on the centre of the screen, but sometimes it is not so optical.
This modification adds one more argument for this feature. Thus, the following code is available (I intentionally paste all code to present the necessity of this feature):

(
Window.closeAll;
s.waitForBoot {
	var sigs = {
		[
			SinOsc.ar(440),
			(SinOsc.ar(440 * (1..48)) * (1..48).reciprocal / 16).sum,
			Resonz.ar(Resonz.ar(WhiteNoise.ar, 440, 0.1, 5), 440, 0.2, 5),
			Resonz.ar(
				Resonz.ar(WhiteNoise.ar, 440 * (1..48), 0.2, 5),
				440 * (1..48), 0.2, (1..48).reciprocal
			).sum,
			WhiteNoise.ar
		] * Env.perc(0.01, 0.79, 0.2).ar(Done.freeSelf)
	};
	FreqScope();
	sigs.().size.do { |i|
		{ sigs.()[i] ! 2 }.scope(position: 605@0); // <- look at this!
		{ sigs.()[i] }.plot(0.04, bounds:Rect(230 * i, 355 + 175, 230, 150));
		{ sigs.()[i] }.plot(1, bounds:Rect(230 * i, 355, 230, 150));
		2.wait
	}
}
)

demo video:
https://www.dropbox.com/scl/fi/qj0337im08mqyzcj2k4fx/Screen-Recording-2024-03-06-at-14.03.44.mov?rlkey=k82z5kwoi2w0nj23sy56nfxht&dl=0

Types of changes

  • Documentation
  • New feature

To-do list

  • Code is tested
  • All tests are passing
  • Updated documentation
  • This PR is ready for review

prko added 3 commits March 6, 2024 13:51
documentation update for the method .scope to define the location of scope window
documentation update for the method .scope to define the scope window location
update .scope method to locate scope window position
@mtmccrea
Copy link
Member

mtmccrea commented Mar 7, 2024

Typically a window widget would accept a Rect for it's position/size, as opposed to just a position Point.

@prko
Copy link
Contributor Author

prko commented Mar 7, 2024

@mtmccrea
Yes, but based on my observations, the size of the 'stethoscope' seems to be set to 250 * 250 or 500 * 500. So I think this item would be sufficient.
Anyway, pressing m should change the size of the `scope', but my modification does not reflect this. I will try to find a way.

I also found a problem. I should fix this.

prko added 5 commits March 8, 2024 12:38
added variables to have a singleton plotTreeWindow and a singleton scopeWindowDefined
changed position argument in scope method to bounds
The position arguments for the server scope method and the function scope method are changed to bounds. And the server scope is changed so that it displays only one scope and works fine when users use manually assigned bounds and the default bound value of nil in succession.
has been modified to show only one boundary and to work fine when users are using manually assigned bounds and the default boundary value of nil, one after the other. Also, pressing m will change its size based on user-defined bounds, if any.
position argument for scope method is changed to bounds, so the documentation is updated
@prko
Copy link
Contributor Author

prko commented Mar 8, 2024

@mtmccrea
Thanks! I fixed all the problems!

I also did the same for Server plotTree and Server meter. However, I am not sure if I should upload these changes here or make two other separate PRs.

a demo video:
https://www.dropbox.com/scl/fi/6wzqzldir2kebqjx5jx20/SC-bounds-added-for-plotTree-scope-and-meter.mov?rlkey=8rpxmwrz1et6659e59zrl1fe9&dl=0

@prko prko changed the title adding position argument to .scope method adding the argument bounds to .scope method Mar 8, 2024
prko added 2 commits March 9, 2024 11:53
added bounds argument to the method `.plotTree`
documentation update for the added argument `bounds` for the method `.plotTree`
@prko prko changed the title adding the argument bounds to .scope method Add the argument bounds to the methods .scope and .plotTree Mar 9, 2024
@prko
Copy link
Contributor Author

prko commented Mar 9, 2024

The bounds argument has also been added to the .plotTree method.

The size of s.meter is automatically calculated by the number of input and output channels. So I think it would be better to give a point argument instead of bounds. However, I will wait for a response from other contributors for a few days. @mtmccrea What do you think?

Copy link
Member

@telephon telephon left a comment

Choose a reason for hiding this comment

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

The idea is reasonable, please add the documentation.

added point to define the server meter position.
documentation update for the added position argument for .meter method.
@prko
Copy link
Contributor Author

prko commented Mar 9, 2024

@telephon
@mtmccrea
I interpreted the comment and change request from @telephon as an approval of the position argument for the .meter method. So, I added this and updated the documentation. If I misunderstood @telephon 's change request, please let me know!

@prko prko changed the title Add the argument bounds to the methods .scope and .plotTree Add the argument bounds to the .scope and .plotTree methods and the argument position to the .meter method to define their position Mar 9, 2024
@prko prko changed the title Add the argument bounds to the .scope and .plotTree methods and the argument position to the .meter method to define their position Add the argument bounds to the .scope and .plotTree methods to define their bounds; add the argument position to the .meter method to define its position Mar 9, 2024
@mtmccrea
Copy link
Member

mtmccrea commented Mar 9, 2024

Something to consider: what would the code look like to change the bounds of the GUI without the new argument?
If it's just a matter of calling s.scope.bounds_(Rect(1,2,3,4)), s.meter.bounds_(Rect(1,2,3,4)), or similar (can't test atm) then it may not be worth adding?

spaces within curly brackets are added
@prko

This comment was marked as resolved.

@prko

This comment was marked as resolved.

The behaviour of resizing has been corrected. It now retains the position as before if the new width and height do not exceed the screen boundary. If the new height exceeds the screen boundary, the vertical point is modified so that it does not exceed the screen boundary. If the new width exceeds the screen boundary, then the horizontal point is modified so that it does not exceed the screen boundary.
@prko

This comment was marked as resolved.

@prko
Copy link
Contributor Author

prko commented Mar 10, 2024

or similar (can't test atm) then it may not be worth adding?

Now there are three options to change the boundary of scope window:

  • s.scope.window.bounds_(Rect(100, 100, 400, 400))
  • s.scope.view.bounds_(Rect(100, 100, 400, 400))
  • s.scope(bounds: Rect(100, 100, 400, 400)) (available if my modification is approved)

@mtmccrea @telephone
I think my modification is worth adding because it is shorter than the other ways.

However, if you think that adding my modification to .scope is not necessary, I will remove all .scope related changes and just leave the modification to .plotTree and .method.

added bounds for scope argument for Bus
@prko
Copy link
Contributor Author

prko commented Mar 13, 2024

Also added the bounds argument to the scope method for the Bus class. There is no argument explanation in the Bus documentation, so I have not updated the schelp file for the Bus class.

@prko prko requested a review from telephon April 2, 2024 22:50
@mtmccrea
Copy link
Member

mtmccrea commented Apr 21, 2024

Thanks for your patience on this @prko.

I think my modification is worth adding because it is shorter than the other ways.

My hesitation about adding these as arguments to the creation methods is that I could image more features being added to these UIs in the future, which may require arguments more closely related to their functionality, as opposed to generic view parameters, which can be accessed through the the methods you found above.

I think a good middle ground may be to add a new -bounds instance method to Stethoscope. In fact it already has a -size method which takes a single argument for it's (square) dimension. (OT: This should probably be extended to take Size or a second arg that allows setting a width and/or height, but anyway...)

This has the added benefit that no change would be needed to the to Bus:-scope method.

I did a quick implementation of Stethescope:-bounds_:

	bounds_ { arg rect;
		if( window.notNil ) { window.bounds_(rect.asRect) };
	}

So to add to the list of calls you found:

s.scope.window.bounds_(Rect(100, 100, 400, 400)) // current
s.scope.view.bounds_(Rect(100, 100, 400, 400)) // current
s.scope(bounds: Rect(100, 100, 400, 400)) // proposed in this PR
s.scope.bounds_(Rect(100, 100, 400, 400)) // suggested

What do you think?

@mtmccrea
Copy link
Member

Ah, I didn't see that @telephon had started a review... I'm happy to defer to the review on whether my suggestion should be considered. I don't mean to hold up a review that's underway.

@prko
Copy link
Contributor Author

prko commented Apr 22, 2024

So to add to the list of calls you found:

s.scope.window.bounds_(Rect(100, 100, 400, 400)) // current
s.scope.view.bounds_(Rect(100, 100, 400, 400)) // current
s.scope(bounds: Rect(100, 100, 400, 400)) // proposed in this PR
s.scope.bounds_(Rect(100, 100, 400, 400)) // suggested

What do you think?

Wow! For me, the way you suggested is better to read:

s.scope.bounds_(Rect(100, 100, 400, 400)) // suggested

I think @mtmccrea's suggestion could also be applied to .plotTree and maybe even .meter. What do you think?

I will wait for @telephon's reply.

@mtmccrea and @telephon. Please tell me what to do:

  • Remove s.scope(bounds: Rect(100, 100, 400, 400)) and add @mtmccrea's suggestion.
  • Remove all my suggestions in this PR and add @mtmccrea's suggestion to s.scope, s.plotTree and s.meter. I may need help with s.meter as the dimension of s.meter is automatically calculated by the number of in and out channels.
  • Just remove s.scope(bounds: Rect(100, 100, 400, 400)). @mtmccrea adds @mtmccrea's suggestion.
  • Close this PR. @mtmccrea adds @mtmccrea's suggestion to s.cope', s.plotTreeands.meter`.

@mtmccrea
Copy link
Member

mtmccrea commented Apr 23, 2024

Remove all my suggestions in this PR and add @mtmccrea's suggestion to s.scope, s.plotTree and s.meter. I may need help with s.meter as the dimension of s.meter is automatically calculated by the number of in and out channels.

With a quick look at all these methods, I see that their internal handling of the created Window is different between them, so the solution won't be the same for each. It's good to have a consistent approach to setting bounds across all server-related GUIs, but this may not be possible.

In general, we try to avoid adding new arguments if the solution can be achieved through method calls that are just as concise.

The .scope solution I mentioned is fairly straightforward, so this PR could be confined to just that if the other methods require further research/experimentation on your part. (As an aside, the length of your PR title may be an indication that this is trying to accomplish "too much" in one PR or the changes are too disparate/not cohesive.)

@mtmccrea mtmccrea added comp: class library SC class library comp: help schelp documentation labels Apr 25, 2024
@prko
Copy link
Contributor Author

prko commented Apr 29, 2024

@mtmccrea
I will remove

  • the argument bounds in .scope and .plotTree;
  • the argument position in .meter in this PR.

Then I will add the -.bounds_ method to the Stethescope class as you suggested.

Then I will open two new PRs:

  • One PR to add the -.bounds_ method to the Server class in the ServerPlusGUI file.
    The serverTree in the current PR allows only one instance of serverTree, and I think it is useful.
  • A PR to add the -.pos_ method to the Server class in the server-meter.sc file.
    .position_ is too long, so I want to use .pos_.

However, to create two new PRs, .meter and .plotTree should return a GUI element, but currently each returns nothing. As far as I have checked using Find in VScode, there are no code examples that follow other methods after .meter and .plotTree. So I think this is not a problem.

What do you think of this idea?

@mtmccrea
Copy link
Member

mtmccrea commented Apr 29, 2024

Then I will add the -.bounds_ method to the Stethescope class as you suggested.

  1. Yes, I think this PR would only involve adding the Stethescope:-bounds method. That takes care of setting the bounds in the case of aServer.scope, and aBus.scope.

  2. Then, one other PR could take care of the methods which you'd like the add a bounds: argument to. I think this will be more clear and easier to evaluate. From the current PR, those include:

Function:-scope // this returns a Synth
Server:-plotTree // this returns a Server
  1. A separate PR for adding a position argument to Server:-meter (note this method returns a Server). The reason I think this should be seprate is because people might have an opinions to offer on a "position" argument, which is uncommon, and there could be other solutions. You don't necessarily want this kind of discussion to hold up the other, more straightforward changes like the ones above.

And to clarify:

One PR to add the -.bounds_ method to the Server class in the ServerPlusGUI file.

The reason you don't want to do this is because bounds are not a property, either literally or conceptually, of Server. It's a property of GUI elements (which Stethescope is). So reason why adding a bounds as a argument to the other methods is necessary, is because they don't return GUI elements, so you can't add a trailing -bounds_ call to it.

I hope I've summarized your goals for these changes accurately! Let me know if I've misunderstood any of your intentions. And these are just my thoughts, others may have a better way of making all this fit together :)

@prko
Copy link
Contributor Author

prko commented May 1, 2024

I removed almost all changes except for this scope window resize behaviour, and added .bounds_ to Stethoscope.

@prko prko closed this May 1, 2024
@prko prko deleted the topic/add_position_to_scope branch May 1, 2024 06:19
@prko prko restored the topic/add_position_to_scope branch May 1, 2024 06:20
@prko
Copy link
Contributor Author

prko commented May 1, 2024

Oh, sorry! Because I changed the name of the connected branch to this PR, the PR is closed and I can connect this PR and the renamed one. I will open a new PR. Sorry, I am still a beginner in this kind of work....

@prko
Copy link
Contributor Author

prko commented May 1, 2024

@mtmccrea I think I have done everything you mentioned. I just hope my activation is helpful for the development of SC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp: class library SC class library comp: help schelp documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants