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

Added Ability To Filter Combinations and Asynchronous Template Loading #256

Merged
merged 13 commits into from
Jun 27, 2019

Conversation

OrkunKocyigit
Copy link
Collaborator

@OrkunKocyigit OrkunKocyigit commented Jun 13, 2019

This PR aims to increase responsiveness of the result screen in application.

This PR contains following:

  • Adds ability to filter combinations by checking grid size. global_logic.js/filterCombinations.
  • Added ruleMaxSize which checks if you equipped every possible slot of your grid or not.
  • Added ruleMaxSize to result.js and profile.js props.
  • Passes ruleMaxSize to calcCombinations.
  • calcCombinations now counts maximum possible slots in grid via maxSize.
  • Added ability to toggle on/off in profile.js (Defaults: true).
    image
  • Asynchronous data loading for arms in template.js via onDataRequested and onDataObtained methods.
  • Asynchronous data loading for characters in template.js via onDataRequested and onDataObtained methods.

src/translate.js Outdated Show resolved Hide resolved
src/profile.js Outdated Show resolved Hide resolved
src/profile.js Outdated Show resolved Hide resolved
@kei-gbf
Copy link
Collaborator

kei-gbf commented Jun 24, 2019

👍 for api document

  • I wanted type info, is it array of number?
  • Separate utility function and unit test

Type info

@param {number[]} combinations
@param {number} ...

I do not know about the type notation style variants,
jsdoc,esdoc, or flow ...

ESDoc type syntax


  • How about separate "sum" as utility function

(minimum sample)

/**
 * sum([1, 2, 3, 4]) => 10
 */
const sum = (arr) => arr.reduce((total, num) => total + num);

// Hide the implementation detail in the filter logic code.
combinations.filter(combination => sum(combination) === max_size);
  • I assume readers may not familiar to functional programming paradigm.
    • I know "reduce" function use is often a tricky for them.
      • it's being popular in JavaScript too, but it's up to their background.
    • "sum" is a popular utility function, lodash also has sum
    • they can understand what the sum() function do without read the implementation detail.
      just write sample code in function comment, is enough.

in JavaScript, compiler does(or may) not optimize the reduce function calls to flat loop.
if need performance tuning, we can switch the sum() implementation later.

function sum(arr, total=0) {
    for (let num of arr)
        total += num;
    return total;
}

and now sum() is a reusable function, you can make unit test

  • module.exports.sum = sum; export for tests
    • NOT: module.exports.sum = function (arr) { ...
      • Because, we can do if run as TEST then module exports those private function later.
      • but no need to change others, no need wide refactoring now.

@OrkunKocyigit
Copy link
Collaborator Author

OrkunKocyigit commented Jun 24, 2019

👍 for api document

  • I wanted type info, is it array of number?
  • Separate utility function and unit test

I implemented those changes with 7df50b9

@kei-gbf
Copy link
Collaborator

kei-gbf commented Jun 26, 2019

  • any cases which the filtering logic does not fit ?
    • e.g. sort by HP and HP debuff weapon ?
  • Check load old save data

@OrkunKocyigit
Can you give a sample save data which the filter works well.
I think coding status is ok. at the last, we need to test exception cases.

@OrkunKocyigit
Copy link
Collaborator Author

OrkunKocyigit commented Jun 26, 2019

This implementation only filters combination arrays rather than actual arm data. It is not aware of stats of the arms henceforth it won't effect anything on stat side.

Current rule only eliminates combinations that has empty slots. If I have an 6 weapons best grid will be 6 be all of them in single grid no matter what those weapons have. This makes it to motocal ignore those grids.

For a simple example on how filter works, it checks number of weapons it has. Let's say we have 6 weapons. Then it ignores any combination that has 5,4,3,2,1 or 0 weapons because they will be worse off no matter what stats they have. Current motocal does calculate for all of it and there is a lot of them.

Number of Different Weapons Without Filtering With Filtering
1 2 1
2 4 1
3 8 1
4 16 1
5 32 1
6 64 1
7 128 1
8 256 1
9 512 1
10 1024 1
11 2048 11
12 4096 66
13 8192 286
14 16384 1001
15 32768 3003
16 65536 8008
17 131072 19448
18 262144 43758
19 524288 92378
20 1048576 184756

Edit: I write in text but didn't like it so here is a pretty picture.

image

@OrkunKocyigit
Copy link
Collaborator Author

OrkunKocyigit commented Jun 26, 2019

@OrkunKocyigit
Can you give a sample save data which the filter works well.

@kei-gbf
You can load the application WITHOUT any save data and can see the difference.

Without:
image

With:
image

@kei-gbf
Copy link
Collaborator

kei-gbf commented Jun 26, 2019

  • Test print out combinations length and filtered combinations length, the result were:
Number of Different Weapons TotalItr Without filterCombinations() With filterCombinations()
10 1024 1024 1
11 2048 11 11
12 4096 66 66
13 8192 286 286

Seem Motocal already does the filtering when number of different weapon 11+

if (isValidCombination && ((totalItr <= 1024 && num <= 10) || num === 10)) {
    combinations.push(temp);
    maxSize = Math.max(maxSize, num);
}
  • (totalItr <= 1024 && num <= 10) || num === 10)
    totalItr 1024 is when number of different weapons 10. (2**10 ?)
    • I added weapons each count 1 ( minimum 0 maximum 1)

however, no much demerit of the filterCombinations
I think the "Filter" has a potential to be user experience as well as "Sort"
as PR it's ok to Approve.

  • Motocal make "combinations" as array now,
    the current Motocal's hard coded filtering does early filtering.
    • implement combinations as Generator + filter will be more flexible. (future topics)
      filter for generator (not Array.filter) will work better for the filterCombinations concept.
  • filterCombinations can optimize until 1024 iterations (10 different weapons?)

This is NOT change request to support those cases.
just discuss exception cases in the logic. for example, Fill the grid may not always come to the top.
Because that does not assume debuff skill and sorting key

sorthp

kei-gbf
kei-gbf previously approved these changes Jun 26, 2019
Copy link
Collaborator

@kei-gbf kei-gbf left a comment

Choose a reason for hiding this comment

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

  • 武器キャラデータの非同期読込
  • 武器数が少ない時の組み合わせ数のフィルタによる最適化
    • 武器数が多くなってきた場合(11種類以上)のフィルタは既に実装済みでしたが、
      他のフィルタロジックを適応を見据えての拡張性有

@kei-gbf kei-gbf dismissed their stale review June 26, 2019 19:47

confirm about polyfill of async

@kei-gbf
Copy link
Collaborator

kei-gbf commented Jun 26, 2019

@OrkunKocyigit
I did not test in other browser,
it's using "async" keyword, we need polyfill ?

@OrkunKocyigit
Copy link
Collaborator Author

OrkunKocyigit commented Jun 27, 2019

@kei-gbf
Oh that is disappointing. I don't think there is harm leaving that but I'll try to improve and add more rules in the future.
What is minimum browser versions we do aim? I am pretty sure current application without anything requires ES8 at least. If you want to aim anything lower I can add polyfill.

https://caniuse.com/#search=Async%20functions

@kei-gbf
Copy link
Collaborator

kei-gbf commented Jun 27, 2019

I do not have the statistics of user-agent access log. all I know is just
./public_html/index.html said the supported version Chrome latest, iOS 10

the support status looks not bad for me.
Babel transpile them to ES5(or ES6) doesn't it?

  • check Babel support and our babel configuration

I think current status is ok
if we did not have another supported browser which the code fail without polyfill,
we can't check the polyfill work or not. when anyone reported the issue we can add polyfill.


and I have to note, we have three ways to run js code

  • main motocal code (babel + webpack)
  • Storybook use different babel config. (under ./.storybook)
  • Jest unit tests use Node.js engine (not browser)

List of new ECMA features we use now

| | version | |
|--|--|--|--|
| let const | ES2015(ES6) | |
| Promises | ES2015(ES6) | |
| Arrow function | ES2015(ES6) | |
| Default parameters | ES2015(ES6) | |
| for-of loop | ES2015(ES6) | |
| Array.include | ES2016(ES7) | (not syntax) |
| async | ES2017(ES8) | |


https://babeljs.io/docs/en/babel-polyfill

As of Babel 7.4.0, this package has been deprecated in favor of directly including core-js/stable (to polyfill ECMAScript features) and regenerator-runtime/runtime (needed to use transpiled generator functions):

we have now @babel/code 7.4.4

https://github.com/zloirock/core-js#babelpolyfill

Copy link
Collaborator

@kei-gbf kei-gbf left a comment

Choose a reason for hiding this comment

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

review done

@OrkunKocyigit
Copy link
Collaborator Author

OrkunKocyigit commented Jun 27, 2019 via email

@OrkunKocyigit OrkunKocyigit merged commit d0ac0ac into MotocalDevelopers:master Jun 27, 2019
@OrkunKocyigit OrkunKocyigit deleted the faster_results branch July 5, 2019 08:01
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