-
Notifications
You must be signed in to change notification settings - Fork 29
attempting to implement orthographic camera #6
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
base: master
Are you sure you want to change the base?
attempting to implement orthographic camera #6
Conversation
|
To test it locally, i did the following:
And that's it. Now, when you want to test some changes you need to run PS: In order for your fork to work, you also need to add |
|
got it, thank you, I figured there had to be something upstream I was missing... btw you can use
Here I think why I failed is because as you pointed out I also need to modify the intermediate dependency of |
|
|
||
| stateInit: () => ({ | ||
| scene: new three.Scene(), | ||
| camera: new three.PerspectiveCamera(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The camera declaration should remain in stateInit. Otherwise you may run into situations where you try to use it before it's declared, for example by invoking cameraPosition before initialization.
If you need one of the config options you can always get it as input to stateInit:
stateInit: ({ cameraType }) => ({
camera: /* cameraType logic */
...
}) There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/three-render-objects.js
Outdated
|
|
||
| if (state._cameraType === 'perspective') { | ||
| state.camera.aspect = state.width/state.height; | ||
| } else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does all of this code need to be run at initialization? It seems fully duplicated from the component update part so perhaps having it there is sufficient?
src/three-render-objects.js
Outdated
| state.renderer.setSize(state.width, state.height); | ||
| state.camera.aspect = state.width/state.height; | ||
|
|
||
| if (state._cameraType === 'perspective') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it perhaps be cleaner to derive this from the camera object itself, eg:
state.camera.type === 'PerspectiveCamera' ? ...
You would also not need to maintain a spare state._cameraType attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, didn't know this was available, will use
src/three-render-objects.js
Outdated
| state.camera.right = width_ortho / 2; | ||
| state.camera.top = height_ortho / 2; | ||
| state.camera.bottom = height_ortho / -2; | ||
| state.camera.near = THREE_JS_PERSPECTIVE_CAMERA_NEAR_DEFAULT; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears to be the current default near value of orthographic camera, so maybe there's no need to define it. It can always be overridden upstream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
Interesting, thanks for the PR @sirrodgepodge! 👍 I left a few minor comments. We should be able to merge this after cleaning up the code a bit. Would you mind adding something to the docs also? |
|
Responded to all the comments. Have to be honest and say that I still couldn't get a test environment working :( :( :(. Maybe you have and can confirm it works. I need to link |
| camera: new three.PerspectiveCamera(), | ||
| clock: new three.Clock() | ||
| clock: new three.Clock(), | ||
| camera: cameraType === 'perspective' ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this logic if the user specifies anything else other than perspective it will fall back to orthographic, which is a little dangerous. Let's swap that so that perspective is the default fallback:
camera: new three[cameraType === 'orthographic' ? 'OrthographicCamera' : 'PerspectiveCamera']();
src/three-render-objects.js
Outdated
|
|
||
| function updateCamera(camera) { | ||
| if (camera.type === 'PerspectiveCamera') { | ||
| state.camera.aspect = state.width / state.height; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You won't be able to access the encapsulated state in a function outside of the kapsule init or update methods.
|
@sirrodgepodge for testing this functionality you can run |
| state.camera.aspect = state.width / state.height; | ||
| } else { | ||
| const aspect = state.width / state.height; | ||
| const height_ortho = depth * 2 * Math.atan( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
depth is not defined.
src/three-render-objects.js
Outdated
| @@ -375,7 +375,7 @@ const THREE_JS_PERSPECTIVE_CAMERA_FOV_Y_DEFAULT = 50; | |||
|
|
|||
| function updateCamera(camera) { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to pass state if you want to access it.
|
whoops, that was super sloppy, apologies, attempted to clean up here: The only thing I'm unclear on is whether Will give up on fancy |
|
|
||
| const THREE_JS_PERSPECTIVE_CAMERA_FOV_Y_DEFAULT = 50; | ||
|
|
||
| function updateCamera(camera, width, height, depth) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nit, depth could be extracted from camera.position.z eliminating the need for the additional argument.
Hello there!
Attempting to implement orthographic camera (for the purpose of 2D Usage of 3d-force graph for performance benefits) using this SO post:
https://stackoverflow.com/questions/48758959/what-is-required-to-convert-threejs-perspective-camera-to-orthographic
I actually was unable to get a test environment working for this (with the different packages working together for some reason NPM link didn't work), I figure you likely have one readily available to take a quick look.