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

Some maintainability improvements #49

Merged
merged 2 commits into from
Jul 1, 2015
Merged

Conversation

vt5491
Copy link
Contributor

@vt5491 vt5491 commented Jun 11, 2015

Hi Brian,
First off, let me thank you for writing this great little app. I've been using it over the last couple of weeks to learn about three.js, oculus Rift, enterprise JavaScript (AMD, Jasmine etc). I recently got a twiddler 3 (chordic input device), and RiftSketch is a great app to force me to learn the key commands since I cannot see the keyboard when I have the Rift on (I know you have a camera view, but I don't have a mobile camera).

But of course, as I'm sure you're aware, once you start using the app for real, you realize you just got to have such and such feature. For instance, you should be able to move around more, the screen should move with you etc etc. So I would look at the code and try to figure out what to do etc. So I've made a bunch of changes to a sandbox version that I have. However, I wouldn't dare check in any fixes from that branch as I used it as a learning experience and was very sloppy and loose about what I did. And some of the changes may not be appropriate anyway.

But once I got to a semi-good milestone level on that sandbox, I decided I should cherry-pick some of the more basic fixes, while it's still fresh in my mind, and commit them back to the parent. These were just some of the things I would like to see if I were to develop further on the project.

Here's a rundown:

  1. I added the Jasmine BDD suite. I work with Perl mostly, and I started doing TDD years ago, and promised myself to always do unit testing no matter what (it's a habit you have to maintain or else you lose it). I had no idea how to do UT in Javascript. I referred to the O'reilly book "Enterprise Web Development" for reference and guidance, so I didn't just choose a solution out of thin air. The hard part here was getting it to work with require.js (esp. since I was also learning about require.js).

Note: I did not add the jasmine libraries under "bower_components". I assume the user will do a "bower install jasmine" and pull it in themselves. I noticed that you seemed to have checked in source code for the other bower_components though.

  1. I wrote a few basic, but non-trivial specs as a template for future commiters, if they so desire to do uts.

  2. I added "minFilter = THREE.NearestFilter" to all the textures creations to avoid the mesage "Texture is not power of two" that was showing thousands of times on the console. This is something only a developer would notice.

  3. I fixed bug Display editor in front of the user when first loading #47 . For me the editor was appearing behind my 180 deg, not 90 deg to the right as the user states. And yes, it was working for me a while ago too. Seems to have broken with v0.6 of OR (?) Oddly, the screen is in the right place if you don't have an OR. Now, after this fix, the screen and cube are in the right place, but it's behind you if you don't an OR. But since only people with an OR would be using this app, I didn't try to fix that.

And honestly, I just tweaked the code to get it to work. I wish I could say I understand enough about EulerRotations and Quaternions to say I figured it out theoretically. So I apologize if you think it's just botched up or a better solution is more obvious to you. You had a variable in there 'hasVR', that didn't appear to be set anywhere. I removed the distinction between the "hasVR" and doesn't "hasVR" paths anyway, as I didn't see the need.

I don't know if you're even actively maintaining this anymore. If you don't like the idea of BDD, or like the code, that's fine too. I'll just continue hacking my private fork.

But I just wanted to say thanks again for writing such a great piece of code. I consider it a real gem. I learned a lot from working on it.

Regards,

Vince Turner

Added jasmine support and fixed the the "texture not a multiple of
two" problem.  Attempting to fix the initial positioning, but
I want to test running the server on windows.  Thus I need to push
the current state so I can pull it down on windows.
2) Create some non-trivial specs as a sort of template for future
specs
3) Prevent annoying "Texture is not power of two" warning message
showing on the console.
4) Make screen and cube appear in front of user if using an HMD.
Previously, it was appearing behind the user.

In short, some basic fixes to make the code base easier to support.
@brianpeiris
Copy link
Owner

Woah tests! This is great, thanks so much. I'll have to take a second look tomorrow before merging it in, I think I have some un-commited changes that I may need to revisit, but it looks good at first glance.

Now I feel bad for the messy state of the code 😝 I'll have to start cleaning it up.

I'd love to hear more about how you've been using it and what other features you'd like to see. Does the twiddler allow for comfortable coding?

@vt5491
Copy link
Contributor Author

vt5491 commented Jun 11, 2015

Ok cool. Just make sure the initial screen and cube is where it should be when you fire up the app with the OR in place, since that's about the only user-facing change I made. I tested with Mozilla nightly build only, as I cannot get the Chrome nightly to recognize my DK2. I mostly tested with client and server running on Windows. I did do some testing with the server on Linux (and client on windows, since client support on linux is hard to do), but for some reason I kept losing keyboard focus on the app, so I stopped doing it (don't know why this would happen since it runs entirely in the client)

The things I added to my sandbox mostly do with movement. You have some seeds of this in your key bindings already. I like to look at the internal scene from multiple angles and positions. I hijacked some of your key bindings for number manipulation, which I didn't use very much, and did more basic WSAD movement, n/p for up/down, q and e for rotating around y axis. The tricky part was getting the screen to move and rotate with you (the screen is now more like a HUD, than an object in the scene), so you can then make changes from the new perspective. While making the screen a HUD takes away from some of the 3-d presence, it's definitely more useful. I found that you should Alt-shift-e the editor away before moving around anyway. I realized its best to make the control commands modal -- they only take effect when the editor is not visible. This way you don't have to Alt-shift to qualify them.

But making the screen a HUD is what I meant by a "may not be appropriate" as it changes the basic flavor of your app as having the editor as an in-screen object is cool in its own way.

Unfortunately, I made the WASD key movements absolute, not relative. That is to says w/s moves you forward backward in absolute space, not according to your rotation. So if you're rotated 90 deg then pressing w moves you to the "right" not "forward" as you would define it from your new perspective. I really need to fix that.

I created a visible x,y,z grid reference, 3 1-unit lines at 0,0,0 as a visible reference, so you know where you are when you move around.

I added a second "info panel" at the top with some twiddler key bindings, an echo of what you have at the bottom already.

But basically, I think you identified a lot of the issues/enhancements in your "issues" section already .. larger screen, a second screen (where you could put some other code for reference). You definitely need to have a config file, to allow people to customize things. People are very picky about editors, and you'll never be able to get a one size fits all.

I seem to have constant problems with losing keyboard focus, particularly when I hide/un-hide the editor and move around. I keep having to Alt-tab to regain focus. It might also be that the underlying textarea loses focus, because some of the key bindings like Alt-shift-e start doing menu dropdowns in the browser. I also have troubles with the cursor on screen getting out of sync with the cursor in the textarea. I don't know how to fix these problems, as I don't know of what the code could be doing to make this happen.

One useful thing on my sand box is the textArea shows on the browser beneath the main panel. I use this to cut and paste in new text and to save what I've done (and get focus back by clicking on it again). I tried to get this to happen on your base version (have the textarea become visible), but I swear I couldn't do it. It's not a code thing, but an HTML thing I think.

As far as the twiddler goes, I wouldn't recommend it for most people. It's hard to say if it's comfortable because I'm still learning it. It really slows down your coding and you lose your flow.

One of the things I like about RiftSketch is it's reasonably small. It's not an overwhelming amount of code. But yet, it has modularity and some structure and uses a lot of best practices (at least for a javascript program :) ). I looked at some of the other 3-d editors out there, but they're too complicated to get started on hacking them (OTOH, they can have much more functionality). I'd almost be worried about adding too much to RiftSketch and having it lose it's simplicity as a starter app for learning about VR.

Let me know if you have any further questions on the pull request, and keep up the good work.

Vince

brianpeiris added a commit that referenced this pull request Jul 1, 2015
Some maintainability improvements
@brianpeiris brianpeiris merged commit f2c18bf into brianpeiris:master Jul 1, 2015
@brianpeiris
Copy link
Owner

Finally got back to this. Thanks for the great feedback too!

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.

2 participants