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

Adding getSize method to SVGRenderer.js and updated docs #19741

Closed
wants to merge 1 commit into from
Closed

Adding getSize method to SVGRenderer.js and updated docs #19741

wants to merge 1 commit into from

Conversation

shaunco
Copy link

@shaunco shaunco commented Jun 26, 2020

Fixes #19732


}

return {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It should be sufficient to just return _svgWidth and _svgHeight. Please keep the implementation simple.

Copy link
Author

Choose a reason for hiding this comment

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

Unlike WebGLRenderer that defaults values for these, SVGRenderer leaves them undefined. This means that a call to getSize that has never called setSize gets { width: undefined, height: undefined }... which seems unexpected?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why should setSize() be never called? The usual pattern in all examples is to create the renderer and then set its size.

Copy link
Author

@shaunco shaunco Jun 28, 2020

Choose a reason for hiding this comment

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

When render.domElement.parent controls the size of render.domElement through CSS, setSize might not be called until you actually need to update the camera's projection matrix in an animate step. Meanwhile, you might call getSize prior to that while setting up your camera or adjusting zoom or the frustrum.

In the case of WebGLRenderer, _width and _height default to _canvas.width and _canvas.height, respectively. In SVGRenderer, _svgWidth and _svgHeight are left undefined. HTML5 specifies that a canvas defaults to 300x150, and it does the same for svg elements, but the some browsers expand inline svg elements to fill the viewport (width: 100vw; height: 100vh;). Either way, the svg element will have a width and a height and I can change the THREE.SVGRenderer constructor to set:

  _svgWidth = _svg.width.baseVal.value;
  _svgHeight = _svg.height.baseVal.value;

If that would be preferred over the getSize implementation I used, but simply leaving getSize() to return undefined values seems wrong.

Copy link
Author

Choose a reason for hiding this comment

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

Actually, I take it back. The svg element is not yet connected to the dom, so _svg.width.baseVal.value can't be resolved in Chromium browsers, as Chromium defaults to 100%. I could do:

	this.getSize = function () {

		if ( ( _svgWidth === undefined || _svgHeight === undefined ) && _svg.parentElement === undefined ) {

			console.warn( "SVGRenderer: .getsize() can't be called until after setSize or after .domElement has been attached to the dom" );

		}

		return { 
			width: _svgWidth ? _svgWidth : _svg.width.baseVal.value, 
			height: _svgHeight ? _svgHeight : _svg.height.baseVal.value
		};

	};

Copy link
Collaborator

@Mugen87 Mugen87 Jun 28, 2020

Choose a reason for hiding this comment

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

Sorry but you make the code unnecessarily complicated. I can only approve this version:

this.getSize = function () {

	return {
		width: _svgWidth,
		height: _svgHeight
	};

};

In general, the example code does not assume that users fiddle around with CSS in order to size the renderer's canvas.

Copy link
Author

Choose a reason for hiding this comment

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

I don't understand how providing a concise warning when developers misuse an API makes the code "unnecessarily complicated"? There are plenty of examples of users that "fiddle around with CSS in order to size the renderer's canvas", including:

and support for this was added to WebGLRenderer back in 2013: ed00cd6 (and expanded in 2016: 5f78a27 ) in response to #2969

Copy link
Collaborator

Choose a reason for hiding this comment

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

The mentioned guide Three.js Responsive Design and #4903 are from the same dev and I don't share his opinion on this topic. I think it's better if the engine does not require the user to work with CSS.

@mrdoob
Copy link
Owner

mrdoob commented Jun 30, 2020

This doesn't work?

this.getSize = function () {

	return {
		width: _svg.clientWidth,
		height: _svg.clientHeight
	};

};

@mrdoob mrdoob added this to the r119 milestone Jun 30, 2020
@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 6, 2020

Closing in favor of #19802.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SVGRenderer does not implement getSize()
3 participants