programminghistorian / ph-submissions

The repository and website hosting the peer review process for new Programming Historian lessons
http://programminghistorian.github.io/ph-submissions
139 stars 114 forks source link

Review Ticket: Creating Mobile Augmented Reality Experiences in Unity #175

Closed walshbr closed 6 years ago

walshbr commented 6 years ago

The Programming Historian has received the following tutorial on 'Creating Mobile Augmented Reality Experiences in Unity' by @jacobwgreene. This lesson is now under review and can be read at:

http://programminghistorian.github.io/ph-submissions/lessons/creating-mobile-augmented-reality-experiences-in-unity

Please feel free to use the line numbers provided on the preview if that helps with anchoring your comments, although you can structure your review as you see fit.

I will act as editor for the review process. My role is to solicit two reviews from the community and to manage the discussions, which should be held here on this forum. I have already read through the lesson and provided feedback, to which the author has responded.

Members of the wider community are also invited to offer constructive feedback which should post to this message thread, but they are asked to first read our Reviewer Guidelines (http://programminghistorian.org/reviewer-guidelines) and to adhere to our anti-harassment policy (below). We ask that all reviews stop after the second formal review has been submitted so that the author can focus on any revisions. I will make an announcement on this thread when that has occurred.

I will endeavor to keep the conversation open here on Github. If anyone feels the need to discuss anything privately, you are welcome to email me. You can always turn to @ianmilligan1 or @amandavisconti if you feel there's a need for an ombudsperson to step in.

Anti-Harassment Policy

This is a statement of the Programming Historian's principles and sets expectations for the tone and style of all correspondence between reviewers, authors, editors, and contributors to our public forums.

The Programming Historian is dedicated to providing an open scholarly environment that offers community participants the freedom to thoroughly scrutinize ideas, to ask questions, make suggestions, or to requests for clarification, but also provides a harassment-free space for all contributors to the project, regardless of gender, gender identity and expression, sexual orientation, disability, physical appearance, body size, race, age or religion, or technical experience. We do not tolerate harassment or ad hominem attacks of community participants in any form. Participants violating these rules may be expelled from the community at the discretion of the editorial board. If anyone witnesses or feels they have been the victim of the above described activity, please contact our ombudspeople (Ian Milligan and Amanda Visconti - http://programminghistorian.org/project-team). Thank you for helping us to create a safe space.

walshbr commented 6 years ago

Corrected the path to the images for the lesson, which were broken. Thanks for this @jacobwgreene! For this review, keep in mind that this is a rewriting of an older lesson that had some sustainability issues. So our primary goal will be to try to make sure that this piece is more sustainably written. Going to reach out to reviewers and then will report back.

jacobwgreene commented 6 years ago

Sounds good! Let me know if you need any help finding potential reviewers. I

walshbr commented 6 years ago

Got one reviewer, and I emailed @jacobwgreene about recommendations for a second. @amsichani is going to shadow the editorial process as well.

jacobwgreene commented 6 years ago

@walshbr I have just gotten in touch with a potential reviewer that I met at conference last week. They study AR from a digital rhetoric perspective but are not familiar with Unity/Vuforia. I'll let you know once I hear back.

walshbr commented 6 years ago

Thanks @jacobwgreene! Loop me in when you get a chance so that I can make sure they are are of the timeline and expectations? I want to make sure they know what they're getting into.

walshbr commented 6 years ago

Just a note to say that @alsalin is going to be one of the reviewers - she is going to take a more sustained look from a sustainability perspective. Shoot me a note when you get a chance @jacobwgreene re: a second reviewer if they've responded. If your person isn't responding we can find another, either by soliciting or putting a call out on Twitter.

jacobwgreene commented 6 years ago

@walshbr I just heard back from my reviewer request, and she has agreed. It is Brenta Blevins, an assistant professor of writing and digital studies at UMW. Here email is sblevin2@umw.edu. If that sounds good, I will let her know that you will be in touch. Do you need any other info for now? Or info that I should send to her?

walshbr commented 6 years ago

That's enough @jacobwgreene - I'll write her in a second with the template we use for reviewers. Thanks!

walshbr commented 6 years ago

Our second reviewer is going to be @erhetorician. I've added her to this repo, and she's agreed to the one month timeline for reviews. So I'll get in touch if I haven't heard anything back by late July.

alsalin commented 6 years ago

Just fyi - I should have my review in by mid-July once I'm done with travel.

alsalin commented 6 years ago

Hi all,

Below are my initial thoughts. This is a detailed edit but I am waiting on time on a Unity machine to go through to compare screenshots (my current computer can't handle the install and we have machines with Unity on them, just have to get to them). I wanted to go ahead and send these edits though. Please let me know if anyone has any questions!

walshbr commented 6 years ago

Thanks @alsalin. Just as a reminder to @jacobwgreene - please don't make any changes to the draft until we have both reviews in and I've given a summary of the reviews. That way we can talk down any places in which the two reviews might disagree. Let me know if you have any questions!

jacobwgreene commented 6 years ago

Thanks for the thorough review @alsalin! @walshbr I'm (finally) starting to get setup at my new place in the southwest, so I should be able to get the revisions done soon after the next review is in.

jacobwgreene commented 6 years ago

Also, my new email is jwgreen5@asu.edu. I should be getting updates at this address now, but I thought I'd list here in case you can't get hold of me.

walshbr commented 6 years ago

Just heard from @erhetorician - she's aiming to have the review in sometime in the next couple days.

erhetorician commented 6 years ago

Before the detailed feedback, a few comments on my approach to the review: I walked through this lesson twice using both a 2017 and a 2018 version of Unity using JDK 8 on a Mac running Sierra and on a Mac running El Capitan, respectively. I was able to successfully build AR apps using the 2018 Unity/El Capitan version, but not the 2017 Unity/Sierra version; I am continuing to debug that problem.

I focused first on a general review of the lesson and then a review in relation to sustainability in regards to continuing software changes. In general, I found the lesson a helpful guide for making decisions around AR humanities projects, as well as for developing projects independent of particular AR-delivery software. I also found the lesson relevant to developing humanities projects using these different versions.

I’ve included feedback below on a paragraph-by-paragraph basis, noting either wording suggestions for grammar or to indicate where I thought a user new to Unity might experience confusion. The risk, of course, of adding some of this additional verbiage is that when Unity/Android/Java/Vuforia/Xcode inevitably change, the lesson will likely require revision. As a user new to Unity, I found the lesson’s Unity images and especially the animations very helpful guides, regardless of whether they reflected exact versions of what I saw on screen. I wondered if verbiage early in the lesson needs to stipulate: “This lesson was written using the [following software]; however, different versions may be available and may function.” I found helpful the link in P14 regarding how to support earlier versions: “If you are unable to download Unity 2017.2 or later, consult this archived lesson for earlier versions of Unity.”

Please let me know if my review raises any questions. I’m happy to look at this lesson again on PC (and Linux beta builds?).

• P6: “we”? Since the author will also be updating “About the Author.” • P11: “that users to have internet access”“that users have internet access” or “that users have to have internet access”? • P12: Perhaps note in or below the table the different Hardware Development Requirements for HP Reveal (can be done using a smartphone/tablet using an app or using a website) and Unity/Vuforia (requires more robust hardware/desktop development hardware). • P13: JDK 8. It’s interesting that JDK 8 is end-of-life this year, but JDK 10 does not work with Unity (my testing, ahem, confirmed that). • P15: Would it be helpful to list a non-version specific link to Oracle’s JDK site? • P17: For “Download and install Android Studio” consider adding “and choose the Standard Install.” • P17: Google USB Driver is a Windows setting, not available on Mac. • P. 18: Consider adding for “1. Start the SDK manager after installation” a phrase like “by choosing the Configure option on the first splash screen.” • P20: Consider adding note: On Mac, the user needs to choose New Project before they can access Preferences. • P23-25: Perhaps note that these instructions are Windows-specific: “Click the Browse button next to the JDK field and select the jdk[version number]folder in the C:/Program Files/Java folder. Leave the NDK field blank.” • P27: What type of project should be created at this step: “Open Unity and create a new project.” 2D, 3D, VR? • P28: Should Hierarchy panel be capitalized throughout (versus “Your hierarchy panel”)? • P28: Consider adding to “then strike the “F” key on your keyboard” a phrase like “to ‘find’ the game object.” • P32: “sounds files, etc.” or “sound files, etc”? • P33: In step 2, suggest following pattern of identifying menu first, then which menu option to select (rather than the reverse): “Add an Image Target to your scene by selecting "Vuforia > Image" in the GameObject dropdown menu.” • P33: Step 7: “sand click”  “and click” • P33: On Step 7, note the user needs to choose to Accept the Vuforia license in Unity. • P33: “8. Copy and past”  “8. Copy and paste” • P45: Step 1 places quotation marks around on-screen locations; suggest being consistent about referencing with or without quotation marks and/or adding the quotation mark before Target: click on “Develop” in the top menu and then select Target Manager.” • P45: Step 3 could reference adding the Name and the Width specification before the Tip about width. • P45: For the Mac step, consider adding the phrase “in Dimensions” to “The first number is the image’s width” (otherwise, the first number displayed is the image byte size). • P49: Add bold text: “If your image is given a good augmentability rating (anything between 3-5 stars should work), access the Target Manager page, then select your image and click Download Database.” • P51: Add bold text: “In the dialogue box that pops up, select Unity Editor and Download” • P51: Change “It will be automatically imported into Unity.” to “An Import dialog box will appear in Unity; click Import to finish.”
• P72: Typo: “helfpul” • P78-79: Suggest using consistent capitalization for “Finder.” I lean toward capitalizing Finder for ease of tracing through the steps. • P80: “Go to the search bar within your project hierarchy panel and search for “DefaultTrackableEventHandler” C# script.” • P80: Change “Click to open in Monodevelop” to “Double-click to open in Unity’s Monodevelop” (versus opening in Visual Studio; single-click opened only in Inspector panel). • P80: The 2017 version did not have “#region PROTECTED_MEMBER_VARIABLES” but I had “#region PRIVATE_MEMBER_VARIABLES” so I changed it to “#region PROTECTED_MEMBER_VARIABLES.” • P80 and subsequent paragraphs: Change “write the following script” to “add the following script before the closing brace }” (or immediately at the beginning?).
• P81: The steps I followed were: “Return to Unity. In the Hierarchy panel, select the ImageTarget. In the Inspector panel, locate the Finder field (under DefaultTrackableEventHandler). From the Hierarchy panel, drag the Finder object to the Inspector panel’s Finder object.” Start a new paragraph with “Within your C# script”? • P83: Should “enable the finder game object if it is lost.” be “enable the finder game object if the target image (or scan) is lost.” • P87: “Select Add Current Add Open Scenes.” “Select Add Open Scenes” • P97: Capitalize “android device”? • P97: Add a note that this step is for Windows development systems: “Open the Android SDK manager and ensure that the "Google USB Driver" is installed. • P99: Check quotation marks and spacing. • P99: Add note that if adb devices returns an empty list on Mac and user has EasyTetherUSBEthernet on Mac, this conflicts with adb and must be unloaded from the command line by entering the following: sudo kextunload -v /System/Library/Extensions/EasyTetherUSBEthernet.kext • P100: check the name in step 3 (e.g.?). • P100: Add after step 3, add another step (or continue step 3): scroll to XR Settings and check Vuforia Augmented Reality.

Overall, I think this is a great lesson supporting AR in the humanities.

walshbr commented 6 years ago

Thanks, @erhetorician and @alsalin! I appreciate the thorough and detailed reviews. I think it is worth testing the 2017 Unity/Sierra version while we are at it if possible, if you have the time @erhetorician. But I don't think that necessarily needs to hold up the revision process. I think any changes from that process should be minor enough that @jacobwgreene can incorporate them later. And if we get to the point of being ready to publish this without having been able to test that build, I think it's OK. We can just list the builds that this has been tested on.

@jacobwgreene - I think it's clear that these reviews won't necessarily require too many substantive changes to the text of the lesson. So I think at this point the best step would be for you to incorporate as many of the suggestions as possible. Feel free to ask questions or open discussion about any of them if you'd like (around the utility or not of particular images, for example). I think in this case we'd like to err on the side of more sustainable where possible, given that we've already had to do this update for the lesson once. I think @erhetorician's suggestion for adding a caveat about how the screenshots and lesson were developed for a particular lesson but may work in other cases might be useful. At this point, minus the one caveat about the 2017 Unity/Sierra build testing, I think we can consider the review closed. Let me know if you have thoughts or concerns!

Next step will be incorporating the feedback into revisions. Let me know if anything comes up @jacobwgreene as well as a timeline for when you think you'll be able to make changes.

jacobwgreene commented 6 years ago

Sounds good @walshbr! And thanks again @erhetorician and @alsalin for the thorough reviews. I will get to work on these revisions and will post here once I push the changes.

jacobwgreene commented 6 years ago

@walshbr I've finished my revisions for the lesson. Should I go ahead and push them to the ph-submissions repo?

walshbr commented 6 years ago

Yep! Go for it. If you push them to the repo here I'll take a look, run through everything, and let you know if I run into any problems.

jacobwgreene commented 6 years ago

Changes should be live now

walshbr commented 6 years ago

@erhetorician - I also am trying to run through the lesson on 2017 Unity / Sierra and also having an issue. Curious as to where you're running into problems? I've got everything running up to the last step for getting the thing to test. I can get the overlay to appear in the webcam view. But it's not registering my target image, so the overlay doesn't disappear and the overlay doesn't appear. I'm going to keep playing with this to make sure it's not something I missed. But in the meantime, here were my notes from the rest of the lesson @jacobwgreene as I worked through it:

Also, am I right in thinking we need to update your bio and institutional affiliation? 🎉

I'll keep seeing if I can troubleshoot what's going on for me - I'm tempted to think it is not the lesson's fault.

walshbr commented 6 years ago

Update re: the issue I was having. It looks like when I imported the database for my image target to Vuforia it made let me select it as a dropdown for the imagetarget game object, but it didn't actually activate the database from Vuforia configuration menu. I'm not sure if that's default behavior or not, but it might be a worth a couple notes at the bottom during the "test your scene" section about one or two common issues that people might have like this one. That could easily get out of scope though, so your call!

I've to got to run at the moment, but now that the scene is working correctly I'll try building it out to mobile for the last step tomorrow to finish out this last read through.

jacobwgreene commented 6 years ago

That's odd. I thought that since 7.2 Vuforia automatically loaded and activated all image target databases that were live in the scene, but it might be useful to have a note about checking the "load" and "activate" parameters in case someone made it this far in the lesson with a previous setup.

I'll get started on these revisions. And yes, I have taken a position at Arizona State University! 🎉 Here is a new bio:

"Jacob Greene is an Assistant Professor of English in the Writing, Rhetorics, and Literacies program at Arizona State University. His research and teaching explore how mobile technologies such augmented reality can be used to promote critical interventions into public and private spaces."

walshbr commented 6 years ago

I'd defer to your knowledge of how Vuforia works - it's totally possible that I might have misclicked somewhere or missed a step. Thanks and congratulations on the new position!

jacobwgreene commented 6 years ago

Thanks! I'm really excited to be here.

I have made the revisions and pushed the changes. I don't have access to a mac right now so I'm having some trouble addressing the issue with the jdk connection and "new project" prompt. I believe when I last ran through this on a mac, there were options for jdk and android, but that might have changed. The most common issue is not checking the "Android Build Support" option when installing Unity.

walshbr commented 6 years ago

In case it helps, after opening Unity I don't have access to the Unity preferences menu until I create a new project and it enters the project window. From inside a project I can get to Unity -> preferences -> external tools. And you were right about JDK - I hadn't added the Android build support because I was working with an iPhone. This all looks good! I got it running on my phone as well. Let me know if you had any final changes you wanted to make @jacobwgreene? Otherwise, I think we're ready to push this out whenever you are.

erhetorician commented 6 years ago

@walshbr Your description of the overlay/target image problem is exactly what I encountered. I may have also run into the same configuration situation as you. Of course, in trying to troubleshoot the problem, I've now corrupted my Vuforia installation, so I need to uninstall and reinstall and that's been slowing me down from replicating the original configuration.

jacobwgreene commented 6 years ago

@walshbr I think everything's good on my end!

walshbr commented 6 years ago

Sounds good! I'll go ahead and finalize things. @erhetorician, if you happen to replicate your configuration and find any further recommendations for that edge case that we both stumbled into let me know. It's easy enough to push up a couple additional sentences once the thing is live.

walshbr commented 6 years ago

Live! Thanks for all of your help @erhetorician and @alsalin. Congrats to you @jacobwgreene! Will tweet to publicize momentarily.