search

shadowmage45 / KerbalFoundries2

KerbalFoundries - Continued
19 stars 8 forks source link

localization support #34

Closed yalov closed 5 years ago

yalov commented 5 years ago
yalov commented 5 years ago

@shadowmage45,
not sure how 

WHEEL
{
    mass = 0.04
    load = 0.5
}

relative to 

PART { mass = 0.15 }

and

MODULE
{
    name = KSPWheelBase
    loadRating = 1
    minLoadRating = 0.1
    maxLoadRating = 3
}

and what are loadRating and minLoadRating ? maxLoadRating is (I assume) max load mass (t) before braking

shadowmage45 commented 5 years ago

@yalov the parameters in the WHEEL block effect how the simulation reacts; it does not actually add/remove mass from the part. Basically it determines how much momentum is 'stored' in a wheel when spinning / how much force it takes to spin up the wheel (which is all separate from how much force it takes to move the complete vehicle).

Load rating/min load/max load -- no longer used. Load rating is all calculated automatically... those fields do nothing.

shadowmage45 commented 5 years ago

Not sure that I'm going to be willing to merge this PR. I appreciate the effort, but there are a few problems that would need to be addressed before I would be willing to merge it.

1.) Why are you changing file names? (not going to merge a PR that changes filenames; if it doesn't fix something -- don't do it) 2.) Why are you re-ordering stuff in config files? (not going to merge a PR that re-orders stuff in the configs for no reason; if it doesn't fix something -- don't do it) 3.) Why are you making balance changes in a PR to add localization support? Any such balance changes should be submitted through a separate PR. As it is now... If I wanted to merge the localization support, I would have to accept the balance changes. What if I only wanted one or the other? 4.) Due to the above mentioned problems -- I cannot even tell what this PR is supposed to do. Without the file-level DIFF to tell me exactly what is changing I cannot audit or verify the changes. All I can see is that you are deleting all of the files...

yalov commented 5 years ago

3-4) I can revert this and later make new PR about balancing 1-2) It's how KSPDev Localization tool works. It makes node indentation, and some reordering ... maybe there is some approved by squad order

shadowmage45 commented 5 years ago

I'm not sure what 'KSPDev Localization' tool is. Do you have links to where it can be downloaded, and preferably to some documentation?

Once again, I appreciate the effort you are putting into these, so please don't get discouraged. But I'll have to find some way to do the localization/etc that doesn't completely mangle every single config file (hopefully there are some options/parameters for the localization tool, otherwise it might have to be done by hand).

yalov commented 5 years ago

A diff tool is more helpful now... do not forget to hide whitespace changes — it is also KSPDev Localization tool changes.

If you accept this, I would like in the next commit revert changed file names, what correspond to name field. It's good practice to have a part name equal to a part-config-file name. It's fix possible patch-writer mistakes, and make my future balancing PR easier

yalov commented 5 years ago

https://github.com/ihsoft/KSPDev_LocalizationTool/wiki

shadowmage45 commented 5 years ago

I would suggest that you not continue to spend time on this in its current format.

I will not be accepting PR's that change file-names, without very good cause. As in 'the part is broken unless it is renamed' kind of cause. If it is not broken, don't change it.

I will not be accepting PR's that mangle files randomly, for no reason. If it doesn't need to be changed, don't change it.

Localization can be done easily without so many un-needed changes. I would say it is even easier without all of the extra fluff...

shadowmage45 commented 5 years ago

Apologies if I offended, and I did not mean to discourage or discount the effort you were putting into this PR.

I am going to say, however, that the 'tool' that you linked to and were using is, in my opinion, terrible, and is the sole cause of the reason that I would not accept the PR as submitted. I'm sure that whomever authored the tool finds it useful personally, but they have also put 'personal preference' into its operations and output formatting, which I do not find desirable or useful.

An automated tool to generate translation files? Great. An automated tool that mangles source files? Not so great. Is there a way to use the tool without all of the extra file-changes?

I have created an example of what a 'clean' PR for localization should look like. The only changes are to the specific lines that are being localized. The diff comes across clean, and I can actually tell what is being changed.

https://github.com/shadowmage45/KerbalFoundries2/pull/35/files

yalov commented 5 years ago

thanks, I understand how localization can be made manually.

Later I will describe my balancing changes in the issue section. The most important for me is

shadowmage45 commented 5 years ago

Thanks. Sorry if I seem a bit unreasonable on the file-changes. I fully agree that part config file-names should match the part-name, and I enforce that rule in mods that I have developed (e.g. SSTU).

In this instance, mostly I don't want these file-names changed to match the part names.... because I hate the current part-names. Before I would do any file-name level changes, I would first have to fix all of the part names to be consistent and logical. And as the changes to part-name would be a 'game-breaking' change -- I am extremely hesitant to do so. Perhaps for the KSP 1.5 release the part and file-names can be reworked, but I certainly won't be changing them before that (due to the aforementioned craft/save breakage).

Balance changes -- sure thing; if you spot stuff like that where the values simply don't make sense, feel free to post up an issue ticket or PR to fix the specific problem.