Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

Conversation

@p30arena
Copy link
Contributor

@p30arena p30arena commented May 14, 2019

Description

modified .gitignore to ommit some vscode related directories.
added contours to both android and iOS.
I'm not sure about sanity of the iOS part.

Related Issues

Replace this paragraph with a list of issues related to this PR from the issue database. Indicate, which of these issues are resolved or fixed by this PR. Note that you'll have to prefix the issue numbers with flutter/flutter#.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

modified .gitignore to ommit some vscode related directories.
added contours to both android and iOS.
I'm not sure about sanity of the iOS part.
@p30arena
Copy link
Contributor Author

p30arena commented May 14, 2019

@bparrishMines

These files are not formatted correctly (see diff above):
firebase_ml_vision/android/src/main/java/io/flutter/plugins/firebasemlvision/FaceDetector.java
firebase_ml_vision/ios/Classes/FaceDetector.m

D:\GitHub\p30arena\plugins> pub global run flutter_plugin_tools format
Downloading Google Java Format...
Formatting all .dart files...
CreateProcessW failed 2
Unhandled exception:
ProcessException: The system cannot find the file specified.

@p30arena
Copy link
Contributor Author

p30arena commented May 14, 2019

I had to run the formatters manually

clang-format -i --style=google  .\FaceDetector.m
java -jar path\to\google-java-format-1.3-all-deps.jar --replace .\FaceDetector.java

@p30arena
Copy link
Contributor Author

p30arena commented May 14, 2019

contours should never be null
but I'm getting this error (maybe on iOS side?)

00:07 +18 -1: /tmp/cirrus-ci-build/packages/firebase_ml_vision/test/firebase_ml_vision_test.dart: FirebaseVision FaceDetector processImage [E]                                                         
  NoSuchMethodError: The method '[]' was called on null.
  Receiver: null
  Tried calling: []("allPoints")
  dart:core                                                 Object.noSuchMethod
  package:firebase_ml_vision/src/face_detector.dart 167:53  new Face._.<fn>
  dart:collection                                           new LinkedHashMap.fromIterables
  package:firebase_ml_vision/src/face_detector.dart 164:19  new Face._
  package:firebase_ml_vision/src/face_detector.dart 85:22   FaceDetector.processImage
  ===== asynchronous gap ===========================
  dart:async                                                _AsyncAwaitCompleter.completeError
  package:firebase_ml_vision/src/face_detector.dart         FaceDetector.processImage
  ===== asynchronous gap ===========================
  dart:async                                                _asyncThenWrapperHelper
  package:firebase_ml_vision/src/face_detector.dart         FaceDetector.processImage
  firebase_ml_vision_test.dart 545:49                       main.<fn>.<fn>.<fn>

@p30arena p30arena changed the title [firebase_ml_vision] adds contours [firebase_ml_vision] adds facial contours May 14, 2019
@p30arena
Copy link
Contributor Author

p30arena commented May 14, 2019

I observed that this doubles (13MB to 30MB) the output size of the APK.
Was this the reason that you haven't had implemented contours in this plugin?

@bparrishMines
Copy link
Contributor

bparrishMines commented May 15, 2019

Hi @p30arena

We haven't added this feature yet because we are currently focusing on integration testing. Once we implement those, it will be easier for us to add additional features to the plugin.

Also, to avoid automatically increasing the size of the APK, we would not include the line api 'com.google.firebase:firebase-ml-vision-face-model:17.0.2' in the build.gradle file. ML Kit models are quite large, so we allow the developer to choose what models to add. For example, if you want to use the ImageLabeler we instruct them to add the dependency themselves. README

Edit: Same for the line s.dependency 'Firebase/MLVisionFaceModel' in the podspec

@p30arena p30arena requested a review from bparrishMines as a code owner May 15, 2019 09:35
@p30arena
Copy link
Contributor Author

p30arena commented Jul 2, 2019

@bparrishMines
Hi there!
Isn't this going to be merged any time soon?

Copy link
Contributor

@bparrishMines bparrishMines left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

Sorry for the delay. Everything looks good. I added a single comment.

Can you handle the merge conflicts, update the pubspec.yaml, and update the CHANGELOG? If not, I can also do these for you.

@bparrishMines
Copy link
Contributor

contours should never be null
but I'm getting this error (maybe on iOS side?)

00:07 +18 -1: /tmp/cirrus-ci-build/packages/firebase_ml_vision/test/firebase_ml_vision_test.dart: FirebaseVision FaceDetector processImage [E]                                                         
  NoSuchMethodError: The method '[]' was called on null.
  Receiver: null
  Tried calling: []("allPoints")
  dart:core                                                 Object.noSuchMethod
  package:firebase_ml_vision/src/face_detector.dart 167:53  new Face._.<fn>
  dart:collection                                           new LinkedHashMap.fromIterables
  package:firebase_ml_vision/src/face_detector.dart 164:19  new Face._
  package:firebase_ml_vision/src/face_detector.dart 85:22   FaceDetector.processImage
  ===== asynchronous gap ===========================
  dart:async                                                _AsyncAwaitCompleter.completeError
  package:firebase_ml_vision/src/face_detector.dart         FaceDetector.processImage
  ===== asynchronous gap ===========================
  dart:async                                                _asyncThenWrapperHelper
  package:firebase_ml_vision/src/face_detector.dart         FaceDetector.processImage
  firebase_ml_vision_test.dart 545:49                       main.<fn>.<fn>.<fn>

Are you still running into this error? Could it be from not including the dependency in the app. I'm not sure if you are using the example app or another.

@p30arena
Copy link
Contributor Author

p30arena commented Jul 19, 2019

Thanks for the contribution!

Sorry for the delay. Everything looks good. I added a single comment.

Can you handle the merge conflicts, update the pubspec.yaml, and update the CHANGELOG? If not, I can also do these for you.

I merged the conflicts,
but, please update the pubspec.yaml and CHANGELOG

Thanks

@p30arena
Copy link
Contributor Author

contours should never be null
but I'm getting this error (maybe on iOS side?)

00:07 +18 -1: /tmp/cirrus-ci-build/packages/firebase_ml_vision/test/firebase_ml_vision_test.dart: FirebaseVision FaceDetector processImage [E]                                                         
  NoSuchMethodError: The method '[]' was called on null.
  Receiver: null
  Tried calling: []("allPoints")
  dart:core                                                 Object.noSuchMethod
  package:firebase_ml_vision/src/face_detector.dart 167:53  new Face._.<fn>
  dart:collection                                           new LinkedHashMap.fromIterables
  package:firebase_ml_vision/src/face_detector.dart 164:19  new Face._
  package:firebase_ml_vision/src/face_detector.dart 85:22   FaceDetector.processImage
  ===== asynchronous gap ===========================
  dart:async                                                _AsyncAwaitCompleter.completeError
  package:firebase_ml_vision/src/face_detector.dart         FaceDetector.processImage
  ===== asynchronous gap ===========================
  dart:async                                                _asyncThenWrapperHelper
  package:firebase_ml_vision/src/face_detector.dart         FaceDetector.processImage
  firebase_ml_vision_test.dart 545:49                       main.<fn>.<fn>.<fn>

Are you still running into this error? Could it be from not including the dependency in the app. I'm not sure if you are using the example app or another.

This error is resolved

Copy link
Contributor

@bparrishMines bparrishMines left a comment

Choose a reason for hiding this comment

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

LGTM

I updated the version and added unit and integration tests.

@bparrishMines bparrishMines merged commit ed53cab into flutter:master Jul 23, 2019
mithun-mondal pushed a commit to bKash-developer/archived_plugins that referenced this pull request Aug 6, 2019
julianscheel pushed a commit to jusst-engineering/plugins that referenced this pull request Mar 11, 2020
julianscheel pushed a commit to jusst-engineering/plugins that referenced this pull request Mar 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants