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

Customcontroller #232

Open
wants to merge 24 commits into
base: master
Choose a base branch
from
Open

Customcontroller #232

wants to merge 24 commits into from

Conversation

anhr
Copy link

@anhr anhr commented Apr 29, 2019

For resolving of issue, described in .add( controller ) #4
Now you can use gui.add(...) for adding of custom controller
Example

@anhr anhr mentioned this pull request Apr 29, 2019
Copy link
Contributor

@donmccurdy donmccurdy left a comment

Choose a reason for hiding this comment

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

Thanks!

Could you omit the build/* and package-lock.json files from this? An easy way to revert that would be:

git checkout master -- build

@@ -455,6 +456,8 @@ const GUI = function(pars) {
}
};

GUI.CustomController = CustomController;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not assign this to the GUI object. Instead we can export CustomController from the module, so it can be used as:

import { GUI, controllers } from 'dat.gui';

class FooController extends controllers.CustomController {}

See https://github.com/dataarts/dat.gui/blob/master/src/dat/index.js.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry I don't understand you. I can not include
import { GUI, controllers } from 'dat.gui';
line into webgl_custom_controller.html file.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you aren't using ES modules, it would just be: dat.controllers.CustomController in this case.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks. Now I want to continue researching of your replies and want to rewrite my code

Copy link
Author

Choose a reason for hiding this comment

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

I have removed GUI.CustomController. See Remove GUI.CustomController.
I want to continue researching of your replies and want to rewrite my code

constructor(object, property) {
super(object, property);

object.constructor( this );
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what this line is doing?

Copy link
Author

Choose a reason for hiding this comment

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

My current version of the CustomController class:

/**
 * @class Represents a custom controller.
 * @param {init} callback function for adding of elements into this.domElement
 */
class CustomController extends Controller{
    constructor(init) {
    super({});

    init( this );
    this.custom = true;
  }
}

I have created my own ES module for testing and use it in my Example . The code below is example of user's custom controller class.

import { GUI, controllers } from '../../dat.gui';

export class KnobController extends controllers.CustomController {
	constructor( a, b ) {
		super(function (controller) {

			var button = document.createElement('span');
			button.innerHTML = 'Knob Controller';
			button.title = 'Please press knob';
			button.style.cursor = 'pointer';
			button.style.margin = '0px 2px';
			button.onclick = function (value) {

				alert('Knob Controller ' + ( knobController.a + knobController.b ));

			}
			controller.domElement.appendChild(button);

		});
		this.a = a;
		this.b = b;
		var knobController = this;

	}
}

object,
property,
{
custom: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the custom: true parameter, if we can already check that the object is an instance of CustomController?

Copy link
Author

Choose a reason for hiding this comment

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

Yes. I have removed it

* @instance
*
*/
addCustomController: function(object, property) {
Copy link
Contributor

Choose a reason for hiding this comment

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

To keep this simple, maybe let's only support gui.add( controller ) for now?

Copy link
Author

Choose a reason for hiding this comment

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

The addCustomController function is the easiest way of using of custom controller. It is ready and I do not want to remove it.

Copy link
Contributor

Choose a reason for hiding this comment

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

See #232 (comment) – I think that add can be just as easy, but if not could you share examples showing how to use them both? It doesn't seem to me like we need both, if add can be simplified.

Copy link
Author

Choose a reason for hiding this comment

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

I have removed addCustomController from my code

@@ -65,3 +65,7 @@ The following libraries / open-source projects were used in the development of d
* [Sass](http://sass-lang.com/)
* [Node.js](http://nodejs.org/)
* [QUnit](https://github.com/jquery/qunit) / [jquery](http://jquery.com/)

## Have a job for me?
Please read [About Me](https://anhr.github.io/AboutMe/).
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to add this?

Copy link
Author

Choose a reason for hiding this comment

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

Currently I am looking for a job. Do you want to remove it from README.md? Do you have a job for me?

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately this project is not the right place to put an available-for-hire post, sorry!

Copy link
Author

Choose a reason for hiding this comment

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

ok

@@ -1 +0,0 @@
.dg ul{list-style:none;margin:0;padding:0;width:100%;clear:both}.dg.ac{position:fixed;top:0;left:0;right:0;height:0;z-index:0}.dg:not(.ac) .main{overflow:hidden}.dg.main{-webkit-transition:opacity .1s linear;-o-transition:opacity .1s linear;-moz-transition:opacity .1s linear;transition:opacity .1s linear}.dg.main.taller-than-window{overflow-y:auto}.dg.main.taller-than-window .close-button{opacity:1;margin-top:-1px;border-top:1px solid #2c2c2c}.dg.main ul.closed .close-button{opacity:1 !important}.dg.main:hover .close-button,.dg.main .close-button.drag{opacity:1}.dg.main .close-button{-webkit-transition:opacity .1s linear;-o-transition:opacity .1s linear;-moz-transition:opacity .1s linear;transition:opacity .1s linear;border:0;line-height:19px;height:20px;cursor:pointer;text-align:center;background-color:#000}.dg.main .close-button.close-top{position:relative}.dg.main .close-button.close-bottom{position:absolute}.dg.main .close-button:hover{background-color:#111}.dg.a{float:right;margin-right:15px;overflow-y:visible}.dg.a.has-save>ul.close-top{margin-top:0}.dg.a.has-save>ul.close-bottom{margin-top:27px}.dg.a.has-save>ul.closed{margin-top:0}.dg.a .save-row{top:0;z-index:1002}.dg.a .save-row.close-top{position:relative}.dg.a .save-row.close-bottom{position:fixed}.dg li{-webkit-transition:height .1s ease-out;-o-transition:height .1s ease-out;-moz-transition:height .1s ease-out;transition:height .1s ease-out;-webkit-transition:overflow .1s linear;-o-transition:overflow .1s linear;-moz-transition:overflow .1s linear;transition:overflow .1s linear}.dg li:not(.folder){cursor:auto;height:27px;line-height:27px;padding:0 4px 0 5px}.dg li.folder{padding:0;border-left:4px solid rgba(0,0,0,0)}.dg li.title{cursor:pointer;margin-left:-4px}.dg .closed li:not(.title),.dg .closed ul li,.dg .closed ul li>*{height:0;overflow:hidden;border:0}.dg .cr{clear:both;padding-left:3px;height:27px;overflow:hidden}.dg .property-name{cursor:default;float:left;clear:left;width:40%;overflow:hidden;text-overflow:ellipsis}.dg .c{float:left;width:60%;position:relative}.dg .c input[type=text]{border:0;margin-top:4px;padding:3px;width:100%;float:right}.dg .has-slider input[type=text]{width:30%;margin-left:0}.dg .slider{float:left;width:66%;margin-left:-5px;margin-right:0;height:19px;margin-top:4px}.dg .slider-fg{height:100%}.dg .c input[type=checkbox]{margin-top:7px}.dg .c select{margin-top:5px}.dg .cr.function,.dg .cr.function .property-name,.dg .cr.function *,.dg .cr.boolean,.dg .cr.boolean *{cursor:pointer}.dg .cr.color{overflow:visible}.dg .selector{display:none;position:absolute;margin-left:-9px;margin-top:23px;z-index:10}.dg .c:hover .selector,.dg .selector.drag{display:block}.dg li.save-row{padding:0}.dg li.save-row .button{display:inline-block;padding:0px 6px}.dg.dialogue{background-color:#222;width:460px;padding:15px;font-size:13px;line-height:15px}#dg-new-constructor{padding:10px;color:#222;font-family:Monaco, monospace;font-size:10px;border:0;resize:none;box-shadow:inset 1px 1px 1px #888;word-wrap:break-word;margin:12px 0;display:block;width:440px;overflow-y:scroll;height:100px;position:relative}#dg-local-explain{display:none;font-size:11px;line-height:17px;border-radius:3px;background-color:#333;padding:8px;margin-top:10px}#dg-local-explain code{font-size:10px}#dat-gui-save-locally{display:none}.dg{color:#eee;font:11px 'Lucida Grande', sans-serif;text-shadow:0 -1px 0 #111}.dg.main::-webkit-scrollbar{width:5px;background:#1a1a1a}.dg.main::-webkit-scrollbar-corner{height:0;display:none}.dg.main::-webkit-scrollbar-thumb{border-radius:5px;background:#676767}.dg li:not(.folder){background:#1a1a1a;border-bottom:1px solid #2c2c2c}.dg li.save-row{line-height:25px;background:#dad5cb;border:0}.dg li.save-row select{margin-left:5px;width:108px}.dg li.save-row .button{margin-left:5px;margin-top:1px;border-radius:2px;font-size:9px;line-height:7px;padding:4px 4px 5px 4px;background:#c5bdad;color:#fff;text-shadow:0 1px 0 #b0a58f;box-shadow:0 -1px 0 #b0a58f;cursor:pointer}.dg li.save-row .button.gears{background:#c5bdad url(data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAsAAAANCAYAAAB/9ZQ7AAAAGXRFWHRTb2Z0d2FyZQBBZG9iZSBJbWFnZVJlYWR5ccllPAAAAQJJREFUeNpiYKAU/P//PwGIC/ApCABiBSAW+I8AClAcgKxQ4T9hoMAEUrxx2QSGN6+egDX+/vWT4e7N82AMYoPAx/evwWoYoSYbACX2s7KxCxzcsezDh3evFoDEBYTEEqycggWAzA9AuUSQQgeYPa9fPv6/YWm/Acx5IPb7ty/fw+QZblw67vDs8R0YHyQhgObx+yAJkBqmG5dPPDh1aPOGR/eugW0G4vlIoTIfyFcA+QekhhHJhPdQxbiAIguMBTQZrPD7108M6roWYDFQiIAAv6Aow/1bFwXgis+f2LUAynwoIaNcz8XNx3Dl7MEJUDGQpx9gtQ8YCueB+D26OECAAQDadt7e46D42QAAAABJRU5ErkJggg==) 2px 1px no-repeat;height:7px;width:8px}.dg li.save-row .button:hover{background-color:#bab19e;box-shadow:0 -1px 0 #b0a58f}.dg li.folder{border-bottom:0}.dg li.title{padding-left:16px;background:#000 url(data:image/gif;base64,R0lGODlhBQAFAJEAAP////Pz8////////yH5BAEAAAIALAAAAAAFAAUAAAIIlI+hKgFxoCgAOw==) 6px 10px no-repeat;cursor:pointer;border-bottom:1px solid rgba(255,255,255,0.2)}.dg .closed li.title{background-image:url(data:image/gif;base64,R0lGODlhBQAFAJEAAP////Pz8////////yH5BAEAAAIALAAAAAAFAAUAAAIIlGIWqMCbWAEAOw==)}.dg .cr.boolean{border-left:3px solid #806787}.dg .cr.color{border-left:3px solid}.dg .cr.function{border-left:3px solid #e61d5f}.dg .cr.number{border-left:3px solid #2FA1D6}.dg .cr.number input[type=text]{color:#2FA1D6}.dg .cr.string{border-left:3px solid #1ed36f}.dg .cr.string input[type=text]{color:#1ed36f}.dg .cr.function:hover,.dg .cr.boolean:hover{background:#111}.dg .c input[type=text]{background:#303030;outline:none}.dg .c input[type=text]:hover{background:#3c3c3c}.dg .c input[type=text]:focus{background:#494949;color:#fff}.dg .c .slider{background:#303030;cursor:ew-resize}.dg .c .slider-fg{background:#2FA1D6;max-width:100%}.dg .c .slider:hover{background:#3c3c3c}.dg .c .slider:hover .slider-fg{background:#44abda}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you omit all changes to build/* from this PR? One way to do that would be:

git checkout master -- build

Copy link
Author

Choose a reason for hiding this comment

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

You have asked me to do it twice. I did it twice. I am not sure, I understand you. Is issue resolved now?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you look at the top of the PR, you'll see that it removes 8,800 lines of code from the project – most files in build/* would be deleted if I merged this. Instead, the files in build/* should be left unchanged.

Screen Shot 2019-06-14 at 11 49 18 AM

Copy link
Author

Choose a reason for hiding this comment

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

I see

this.custom = true;
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

From your example

var controllerPlayRate1 = folder1.add( new dat.controllers.CustomController( function ( controller ) {

  controllerPlay( controller, buttons1 );

} ),
{

  playRate: 1,

}, 'playRate', 1, 25, 1 ).onChange( function ( value ) {

  onChangePlayRate( value );

} );

I'd rather have an API where custom controllers can be used more similarly to the builtin controller types. Users would not instantiate CustomController directly (i.e. it's an "abstract" class) but would instantiate subtypes. For example, they should do this:

class KnobController extends CustomController {
  constructor ( object, name, ...opts ) {
    super( object, name );
    // ... set up options if needed
  }
}

const api = {
  color: '#ffffff',
  value: 0.5
};

const gui = new dat.GUI();
gui.add( api, 'color' );
gui.add( new KnobController( api, 'value' ) );

I believe that can be implemented without requiring an init callback, and without needing a addCustomController method (since add works just as well).

Copy link
Author

Choose a reason for hiding this comment

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

Please see my new version of the CustomController.js as you asked me. I have rewired my own ES module for testing and use it in my example.

anhr added a commit to anhr/customController that referenced this pull request Jun 17, 2019
API where custom controllers can be used more similarly to the builtin
controller types.
dataarts/dat.gui#232 (comment)
anhr added a commit to anhr/customController that referenced this pull request Jun 17, 2019
API where custom controllers can be used more similarly to the builtin
controller types.
dataarts/dat.gui#232 (comment)
anhr added a commit to anhr/customController that referenced this pull request Jun 17, 2019
API where custom controllers can be used more similarly to the builtin
controller types.
dataarts/dat.gui#232 (comment)
API where custom controllers can be used more similarly to the builtin
controller types.
dataarts#232 (comment)
This was referenced Jun 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants