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

fps calculation #42

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

fps calculation #42

wants to merge 4 commits into from

Conversation

WolfgangFahl
Copy link

this branch calcs and shows fps (in the log)

@xSAVIKx
Copy link
Owner

xSAVIKx commented May 21, 2017

@WolfgangFahl, sorry, nothing useful for the project again.

If you know how to improve FPS - I'd appreciate the PR. But please follow the coding standards of the project and be sure not to make Codacy rate worse.

@WolfgangFahl
Copy link
Author

If I were you I'd put more focus on the product quality than the process quality.
You might want to use the few line of fps calculation and add them in a way that the fps gets displayed. Then it would be useful to be able to control it e.g. via the userinterface or command line.
Also it would be great to be able to test the (currently platform dependent) adb handling. Probably there is no use to try this on travis because its too much hassle or not feasible to get the device emulation working in a docker container that travis uses.

Personally I find the rejection of the pull request frustrating. You might want to be more inviting to potential contributors.

@xSAVIKx
Copy link
Owner

xSAVIKx commented May 21, 2017

@WolfgangFahl sorry if it sounds a bit harsh, but it's open source - everyone is able to submit PR, but not all the code would be accepted.

If you want to add any additional functionality - you're welcome, just fork the project and implement everything you want in a way you want it to be implemented.

Unfortunately I cannot spend all my free time working on this project, but I do what I can.

For the FPS calculation - for sure it's useful, but not in the way you have implemented it right now. I'm not closing the PR, so you are free to update your solution.

For now, I don't think I'd merge this into the project for the following reasons:

  1. you have added some weird comments
  2. the bash script, you've added is platform-dependent, while project should be built on any platform.
  3. you've added unit test, that does not test anything.

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