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

Unit DB Feature parity PR #57

Closed
wants to merge 11 commits into from

Conversation

Raistlfiren
Copy link

@Raistlfiren Raistlfiren commented Nov 22, 2016

This is currently a placeholder PR as I work on updating code to fix weapons, add DoTPulses, fix fire cycle, add more weapon details, add link to Github blueprint, and add stat highlighting.

This should knock out the following issues - https://github.com/spooky/unitdb/issues/56, https://github.com/spooky/unitdb/issues/54, https://github.com/spooky/unitdb/issues/53

Current todo list:

  • Fix fire cycles
  • Add DoTPulses
  • Add Firing Randomness
  • Add Firing Lifetime
  • Fix T3 aeon rapid fire artillery
  • Fix beam fire cycle
  • Add beam cycle
  • Link unit to Github blueprint
  • Fix miscellaneous weapon issues

@Raistlfiren
Copy link
Author

@spooky - Please review the PR when you get a chance. I have fixed all of the problems with weapons now, and I have added more information about beams. I also added a link from the blueprint ID to the specific Github page for easy reference. (This might need to be more obvious than what it currently is.)

As an aside, I would like to link/include this unit database as the main database now on the new website. Do you have any problems with connecting this repository to the official Faforever organization? Or at least me adding a Dockerfile so it can be deployed on the Faforever servers?

@spooky
Copy link
Collaborator

spooky commented Dec 6, 2016

@Raistlfiren thanks for the PR, will need a bit more time to review/merge though. As for connecting this to faforever & linking/installing/sharing - no problems with that, this is open source and is meant to serve the community. Connecting or trasfering the repo to faforever would require me haivng admi permissions in that organization (https://help.github.com/articles/transferring-a-repository-owned-by-your-personal-account/)

@Raistlfiren
Copy link
Author

@spooky - thank you for taking the time to review the PR. I will try to discuss the merge with downlord and sheeo.

@Raistlfiren Raistlfiren changed the title [WIP] Unit DB Feature parity PR Unit DB Feature parity PR Dec 6, 2016
@Raistlfiren
Copy link
Author

@spooky - After speaking with downlord on slack, he said that we can transfer the repository to the organization and you would still have write access to the repository at first. Admin access would need to be explicitly given to you by visionik or sheeo. I will try to reach out to them as well.

@Raistlfiren
Copy link
Author

@spooky - I just wanted to check in and see if you have been able to review it and think about moving to the Faforever organization. Downlord should be able to help you with the transition. If you would like to talk further, then we can discuss it on slack.

@spooky
Copy link
Collaborator

spooky commented Jan 12, 2017

@Raistlfiren will do, just can't seem to find the time. When I do, I'll catch someone on slack and try to push this further.

Copy link
Collaborator

@spooky spooky left a comment

Choose a reason for hiding this comment

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

I've put comments near the places that need to be fixed

Copy link
Collaborator

@spooky spooky left a comment

Choose a reason for hiding this comment

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

some changes are needed.

@@ -398,12 +398,12 @@ def run(app_path):
unit_data_path = os.path.join(app_path, 'data', 'index.json')
unit_dataFat_path = os.path.join(app_path, 'data', 'index.fat.json')

unit_list = load_unit_file(unit_data_path)
unit_list = load_unit_file(unit_dataFat_path)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this commit is incorrect, though I understand the confusion. The way this works is that first the file called (by default) index.json is generated by running blueprint2json.lua so that file is actually the complete blueprint db (no filtering). Then the cleaner is run and that's what adjusts the index.json to a format understood by the app. So as misleading as it seems, the previous version of the code is correct.
As an improvement it would be nice to create a config file containing all the paths - that should clear up the confusion

Copy link
Author

Choose a reason for hiding this comment

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

Well, the problem is that the method slenderize is being called AFTER the file is loaded. I was running into issues where some of the new properties like FiringRandomness, FiringTolerance, and etc... wasn't being added to my index.json file because the index.json file only included the slimmed down version of properties. Unless I am still not clear about this, please verify. Thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Slenderize is ment to throw out all the properties that are not used so that the index is smaller in size. The issue you need to solve is why slenderize doesn't find the new properties.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, exactly. The problem was that index.json did not include the methods and so they were not being included in the updated output. Once I changed that to the fat JSON file, then I was able to get all of the new methods and include them in the outputted file - index.json. The methods weren't included because they were never included initially.

app/css/main.css Outdated
@@ -171,7 +171,7 @@ footer {
}
.faction {
float: left;
width: 25%;
/*width: 25%;*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

this broke the layout when a filter is applied. Check what happens when you click any of the class filters (e.g navy) or type in a unit name


return unitDb.dpsCalculator.dps(weapon, stats);
},
isTML = function(weapon) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

isTML = function(weapon) { return !!weapon.ForceSingleFire; }

if (weapon.ForceSingleFire)
return true;

return false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

there's a lot of dense code in this file - this should be extracted to smaller methods - right now it's really hard to follow.

test/spec/dps.js Outdated
@@ -27,7 +27,8 @@ describe('dps', function () {
'TurretYawSpeed': 100,
'WeaponCategory': 'Direct Fire'
};
var dps = unitDb.dpsCalculator.dps(weapon);
var stats = JSON.parse('{"shots":1,"cycle":0.3,"damage":8,"salvoDelay":0,"salvoSize":1,"dotPulse":1}');
Copy link
Collaborator

Choose a reason for hiding this comment

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

JSON.parse is really not needed here... this is js, you can just create the object directly. That goes for all the JSON.parse calls that have been added in this file.

Copy link
Author

Choose a reason for hiding this comment

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

This is silly. I don't really know why I did this. Thanks for the catch and I will update my PR soon with these changes.

@spooky
Copy link
Collaborator

spooky commented Nov 4, 2017

merged changes in 931f00b

@spooky spooky closed this Nov 4, 2017
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