CAT-1316 Sensor-type to GAMING; LIN.ACC.- and ROT.VECT. (inclin.,… #1325

ghost · 2015-08-03T14:21:41Z

…compass-dir) sensor fallback

aried3r · 2015-08-03T20:17:15Z

catroid/src/org/catrobat/catroid/formulaeditor/SensorHandler.java

-	private float linearAcceleartionX = 0f;
-	private float linearAcceleartionY = 0f;
-	private float linearAcceleartionZ = 0f;
+	private float[] accelerationXYZ = new float [3];


Extraneous space between float and [3].

aried3r · 2015-08-03T20:20:49Z

Please run the formatter on your files, there's quite a few formatting errors.

martinbu · 2015-08-04T15:38:39Z

Everything works fine now on Moto G1.

ghost · 2015-08-04T20:40:40Z

@aried3r: formatter run. Didn't think of it - but last time I ran the formatter @obusher had something against it:

obusher commented on May 18
I know the format of this class is not in best shape and it is compelling to use auto-format.
But i want to see a clean github-diff, so please do not use this magic. 🐰

I generally understand that - but it wouldn't be a problem when the formatter would be run automagically in every Android Studio on save - so the format wouldn't deteriorate in the first place.

@martinbu: Thanks for your help finding my copy/paste error - now the values in the Compute-Dialog in the formula-editor are correctly updated 👍

aried3r · 2015-08-04T20:59:42Z

While I agree that there should be an option to format it automatically, there is an option to only format the lines you have changed.

And while I don't personally commit from within Android Studio, there is an option to format before committing.

robertpainsi · 2015-08-04T21:42:15Z

edit: Dangle-it, @aried3r was faster! 😔

JFI: The comment you quoted is outdated and all java classes (except two lines [1] and [2]) are formatted according to the Android Studio formatter.
All new checkstyle rules are doing a great job, however, the approach mentioned by @aried3r should be used/preferred.

ghost · 2015-08-04T22:12:20Z

@aried3r : that's cool thanks - haven't tried committing from Android Studio yet.

aried3r · 2015-08-04T22:18:47Z

All new checkstyle rules are doing a great job

Is there a way to have Jenkins run this and report back within a few minutes? If this isn't already the case. I don't want to comment every PR because of these things.

It seems GitHub can list multiple test runs on PRs. See here for an example.

robertpainsi · 2015-08-05T10:56:00Z

Is there a way to have Jenkins run this and report back within a few minutes? If this isn't already the case. I don't want to comment every PR because of these things.

There is already an experimental jenkins job waiting to replace the current Catroid-Build-SourceTests-StaticChecks job. The new one will abort the complete PR testrun if there are any pmd, checkstyle or lint warnings. So, if a jenkins testrun takes only a few minutes, which you can check even without a jenkins account, it highly indicates new static code analyzing warnings. I'll ping you if the new job has been activated.

It seems GitHub can list multiple test runs on PRs. See here for an example.

Thanks for the tip! I like it and I'll write a (low priority) ticket for it. 😸

thmq · 2015-08-07T11:15:48Z

catroid/src/org/catrobat/catroid/formulaeditor/SensorHandler.java


 			case COMPASS_DIRECTION:
 				float[] orientations = new float[3];
-				android.hardware.SensorManager.getRotationMatrixFromVector(instance.rotationMatrix,
-						instance.rotationVector);
+				if (!instance.useRotationVectorFallback) {


what happens here if useRotationVectorFallback is true? correct me if I'm wrong, but if there is no getRotationMatrixFromVector then the rotationMatrix is not initialised and hence you won't get any reasonable results anyway.

Hi!
If the useRotatitonVectorFallback is true - then the rotatiton-matrix is NOT obtained from the RotationVectorSensor (because this does not exist in this phone) but determined by means of the getRotationMatrix(float[],float[],float[],float[]) with accelerometer data AND macgnetic field data in line 303 in the onSensorChanged(...) method - when the MagneticField Sensor is present.
If NEITHER rotation-matrix sensor NOR magnetic field sensor are present compass direction does not work on this phone.

all right! got it! obviously I hadn't understood it properly before, thank you for clearing that up.

thmq · 2015-08-07T11:19:34Z

apart from the question regarding the compass direction the PR looks fine. any objections against merging it as soon as this has been cleared up? @robertpainsi @martinbu ?

thmq · 2015-08-07T11:22:53Z

catroid/src/org/catrobat/catroid/formulaeditor/SensorHandler.java

+					float rawInclinationX = RADIAN_TO_DEGREE_CONST * (float) (Math.acos(instance.accelerationXYZ[0]));
+					float correctedInclinationX = 0;
+
+					// X-inclination: turn right direction


I don't think the comments are necessary.

ghost · 2015-08-07T12:03:22Z

removed the two comments

thmq · 2015-08-07T12:20:28Z

retest this please

robertpainsi · 2015-08-07T12:56:51Z

catroid/src/org/catrobat/catroid/formulaeditor/SensorHandler.java

+							correctedInclinationX = -(180 + (90 - rawInclinationX));
+						}
+					} else
+						if (rawInclinationX >= 0 && rawInclinationX < 90) {


That's an interesting case of else-if coding style...
Do you read it as else { if (... (because a else block doesn't require braces) or as else if (...?
However, let's see what the android studio formatter does! 😉

Also removing comments, as suggested by @thmq, does not always end up by just removing them. Please take a look at Clean Code, "Don't use a comment when you can use a function or variable".

However, I prefer this example

// Bad: // Check to see if the employee is eligible for full benefits if ((employee.flags & HOURLY_FLAG) && (employee.age > 65)) // Good: if (employee.isEligibleForFullBenefits())

thmq · 2015-08-07T14:36:34Z

@robertpainsi 's right about this. you should try to keep your code as well legible as possible. besides there seems to be a problem with the sensor test [1].

avihric · 2015-08-07T14:54:41Z

retest this please

…ass-dir) sensor fallback

ghost · 2015-08-07T20:40:37Z

@thmq damn this test uses "reflection"!
I was sure the test was green but... since the "accelerometerSensor" which was originally used in the SensorHandler was actually the "linearAccelerationSensor" AND
"linearAcceleration[X|Y|Z]" were misspelled as "linearAcceleartion[X|Y|Z]" ;-)

So this happens when correcting variable-names AFTER running the "final" tests which uses "reflection". 🙀 Shame on me that I didn't have a look at the test - so I would have known.

avihric · 2015-08-08T09:03:21Z

I will merge this because the Release is urgent, but you will have to refactor Sensorhandler as described by @robertpainsi

Btw: I will cherry-pick this commit into master

CAT-1316 Fix sensor issues

aried3r reviewed Aug 3, 2015
View reviewed changes

ghost force-pushed the CAT-1316 branch from 71e8514 to e6cc802 Compare August 4, 2015 20:03

ghost force-pushed the CAT-1316 branch 2 times, most recently from 164e46b to 3c0530e Compare August 5, 2015 20:11

thmq reviewed Aug 7, 2015
View reviewed changes

ghost force-pushed the CAT-1316 branch from 3c0530e to 88ec2b4 Compare August 7, 2015 12:01

robertpainsi reviewed Aug 7, 2015
View reviewed changes

CAT-1316 Sensor-type to GAMING; LIN.ACC.- and ROT.VECT. (inclin.,comp…

2045020

…ass-dir) sensor fallback

ghost force-pushed the CAT-1316 branch from 88ec2b4 to 2045020 Compare August 7, 2015 20:41

avihric added a commit that referenced this pull request Aug 8, 2015

Merge pull request #1325 from amix73/CAT-1316

3601b6a

CAT-1316 Fix sensor issues

avihric merged commit 3601b6a into Catrobat:develop Aug 8, 2015

avihric mentioned this pull request Aug 8, 2015

CAT-1443 Set/change variable fixed #1306

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CAT-1316 Sensor-type to GAMING; LIN.ACC.- and ROT.VECT. (inclin.,… #1325

CAT-1316 Sensor-type to GAMING; LIN.ACC.- and ROT.VECT. (inclin.,… #1325

ghost commented Aug 3, 2015

aried3r Aug 3, 2015

aried3r commented Aug 3, 2015

martinbu commented Aug 4, 2015

ghost commented Aug 4, 2015

aried3r commented Aug 4, 2015

robertpainsi commented Aug 4, 2015

ghost commented Aug 4, 2015

aried3r commented Aug 4, 2015

robertpainsi commented Aug 5, 2015

thmq Aug 7, 2015

ghost Aug 7, 2015

thmq Aug 7, 2015

thmq commented Aug 7, 2015

thmq Aug 7, 2015

ghost Aug 7, 2015

ghost commented Aug 7, 2015

thmq commented Aug 7, 2015

robertpainsi Aug 7, 2015

thmq commented Aug 7, 2015

avihric commented Aug 7, 2015

ghost commented Aug 7, 2015

avihric commented Aug 8, 2015

CAT-1316 Sensor-type to GAMING; LIN.ACC.- and ROT.VECT. (inclin.,… #1325

CAT-1316 Sensor-type to GAMING; LIN.ACC.- and ROT.VECT. (inclin.,… #1325

Conversation

ghost commented Aug 3, 2015

aried3r Aug 3, 2015

Choose a reason for hiding this comment

aried3r commented Aug 3, 2015

martinbu commented Aug 4, 2015

ghost commented Aug 4, 2015

aried3r commented Aug 4, 2015

robertpainsi commented Aug 4, 2015

ghost commented Aug 4, 2015

aried3r commented Aug 4, 2015

robertpainsi commented Aug 5, 2015

thmq Aug 7, 2015

Choose a reason for hiding this comment

ghost Aug 7, 2015

Choose a reason for hiding this comment

thmq Aug 7, 2015

Choose a reason for hiding this comment

thmq commented Aug 7, 2015

thmq Aug 7, 2015

Choose a reason for hiding this comment

ghost Aug 7, 2015

Choose a reason for hiding this comment

ghost commented Aug 7, 2015

thmq commented Aug 7, 2015

robertpainsi Aug 7, 2015

Choose a reason for hiding this comment

thmq commented Aug 7, 2015

avihric commented Aug 7, 2015

ghost commented Aug 7, 2015

avihric commented Aug 8, 2015