[1.12.x] SXT Continued

[voicey99]

Going through and making tweaks to quite a lot of partfiles in addition to entering atmosphere curves (correcting description typos/grammar/spelling, removing superfluous lines, adding spacing to node contents etc.), and one of them had this notice in:

    // ----- DO NOT EDIT BELOW THIS POINT ------
    // --- I'll do what I want thank you very much ---

Does this rather curt message mean you're not supposed to touch the lower half of the file at all for whatever reason, or is it just a relic?

Edited November 8, 2017 by voicey99

[Kevin Kyle]

If you were attempting to fix the TT-06 launch tower I thank you for your efforts but I have no clue what exactly that means and depending on what its licensed under, it might prevent you from doing that but I dont really understand all of that stuff anyway. Its all very confusing. I looked through all the files just trying to figure out which file it was exactly. I did find a launch tower 1 in there that didnt have a config file, I wondered about that. I so wish I knew more.

[voicey99]

1 hour ago, Kevin Kyle said:

If you were attempting to fix the TT-06 launch tower I thank you for your efforts but I have no clue what exactly that means and depending on what its licensed under, it might prevent you from doing that but I dont really understand all of that stuff anyway. Its all very confusing. I looked through all the files just trying to figure out which file it was exactly. I did find a launch tower 1 in there that didnt have a config file, I wondered about that. I so wish I knew more.

No, it's the Boconok-9 (the launch clamp doesn't have the notice). I'm going round the partfiles removing whitespace and adding some proper tab spacing where needed to structure the config more neatly - I removed a couple of blank lines and added some spacing anyway given it doesn't actually alter the part in any way. If necessary I can just not include it in the commit, unless Linux can tell us more?

[linuxgurugamer]

6 minutes ago, voicey99 said:

No, it's the Boconok-9 (the launch clamp doesn't have the notice). I'm going round the partfiles removing whitespace and adding some proper tab spacing where needed to structure the config more neatly - I removed a couple of blank lines and added some spacing anyway given it doesn't actually alter the part in any way. If necessary I can just not include it in the commit, unless Linux can tell us more?

What's the question?

[voicey99]

1 minute ago, linuxgurugamer said:

What's the question?

See the top of the page.

[linuxgurugamer]

3 hours ago, voicey99 said:

Going through and making tweaks to quite a lot of partfiles in addition to entering atmosphere curves (correcting description typos/grammar/spelling, removing superfluous lines, adding spacing to node contents etc.), and one of them had this notice in:

    // ----- DO NOT EDIT BELOW THIS POINT ------
    // --- I'll do what I want thank you very much ---

Does this rather curt message mean you're not supposed to touch the lower half of the file at all for whatever reason, or is it just a relic?

It's a relic, it is NOT a license

[linuxgurugamer]

I have a test release for people to examine here:

https://github.com/linuxgurugamer/SXTContinued/releases/tag/0.3.17.-1

The changes were submitted by @voicey99.  Unfortunately for me, he changed 188 files,which I don't have time to go through right now.  So I made a branch and merged his changes into the branch.

I'd appreciate some other eyes to take a look at this

[voicey99]

14 minutes ago, linuxgurugamer said:

I have a test release for people to examine here:

https://github.com/linuxgurugamer/SXTContinued/releases/tag/0.3.17.-1

The changes were submitted by @voicey99.  Unfortunately for me, he changed 188 files,which I don't have time to go through right now.  So I made a branch and merged his changes into the branch.

I'd appreciate some other eyes to take a look at this

The vast majority of the changes were just me removing redundant lines and redoing spacing with no modifications to the actual config, and the intended changes are documented in the PR. It looks like my changes to improve readability were rendered totally moot by a conversion to horizontal format over the vertical format they were in in the GitHub download, which means they are very difficult to read and follow anyway.

If anyone finds an error I made (e.g. truncated lines by mistake), let me know and I'll fix it asap.

[linuxgurugamer]

4 minutes ago, voicey99 said:

The vast majority of the changes were just me removing redundant lines and redoing spacing with no modifications to the actual config, and the intended changes are documented in the PR. It looks like my changes to improve readability were rendered totally moot by a conversion to horizontal format over the vertical format they were in in the GitHub download, which means they are very difficult to read and follow anyway.

If anyone finds an error I made (e.g. truncated lines by mistake), let me know and I'll fix it asap.

That's actually because of the format, I'm pretty sure they are in Unix format, which ends lines with a CR, while Windows uses CR-NL

You can use Notepad++ or GVim to view it properly on Windows

If you do make changes, pull down the branch and make PRs against the branch, will make it easier for me.

Edited November 9, 2017 by linuxgurugamer

[swjr-swis]

42 minutes ago, linuxgurugamer said:

I'd appreciate some other eyes to take a look at this

I have a few hours to burn... scrolling through them right now.

So far (about 20% in) I have seen only whitespace, line order, and spelling changes to description text, none of which affect the functionality of the files.

A completely subjective and perfectly ignorable opinion if I may express one: personally, I prefer a line of whitespace between NODE{} sections, which I notice have been removed with prejudice in this PR. But like I said, it changes nothing (so far) to the functionality.

I'll report back in again as soon as I've worked my way through.

[linuxgurugamer]

4 minutes ago, swjr-swis said:

I have a few hours to burn... scrolling through them right now.

So far (about 20% in) I have seen only whitespace, line order, and spelling changes to description text, none of which affect the functionality of the files.

A completely subjective and perfectly ignorable opinion if I may express one: personally, I prefer a line of whitespace between NODE{} sections, which I notice have been removed with prejudice in this PR. But like I said, it changes nothing (so far) to the functionality.

I'll report back in again as soon as I've worked my way through.

So do I, but that's easy enough to put back

[voicey99]

3 hours ago, linuxgurugamer said:

That's actually because of the format, I'm pretty sure they are in Unix format, which ends lines with a CR, while Windows uses CR-NL

In the raw download I used to modify it, they were written CR-NL but in the release they are CR. I guess you using Unix-like Linux (if your name is anything to go by) converts them.

2 hours ago, linuxgurugamer said:

So do I, but that's easy enough to put back

In the majority of files this was very inconsistent, with some nodes having a space and others not. A large majority of nodes did not have a space, hence I decided to unify it that way instead. I can change it back the other way if you want, but it does make the files noticeably longer.

[linuxgurugamer]

1 minute ago, voicey99 said:

In the majority of files this was very inconsistent, with some nodes having a space and others not. A large majority of nodes did not have a space, hence I decided to unify it that way instead. I can change it back the other way if you want, but it does make the files noticeably longer.

I prefer a blank line in front of the sections, so if you can do that, that would be great.

[swjr-swis]

Still a bit to go, unfortunately not sure how much because I hadn't noticed that github doesn't load the entire set of diffs at once... I thought I was nearing the end before I ran into 'Load diff' which I have to do one by one.

So I took a break for a moment to gather a bit of energy to go through the rest.

Results so far: no errors found that would be caused by the diffs, so that's good. I did encounter a few functional changes, in all cases atmosphere curves - most of them extended into the higher pressure range, one had some tweaking on the pre-existing points. I can't judge just from looking at the text how the adaptations would work, but they look ok and in line with the advertised purpose.

Back to see if I can finish the rest of it.

[voicey99]

2 hours ago, linuxgurugamer said:

I prefer a blank line in front of the sections, so if you can do that, that would be great.

Ok, I went through and added a line between every top-level module, parameter category and their titles. There's now more spacing than there was before I took a hatchet to it. It's in a PR to the new branch.

2 hours ago, swjr-swis said:

Results so far: no errors found that would be caused by the diffs, so that's good. I did encounter a few functional changes, in all cases atmosphere curves - most of them extended into the higher pressure range, one had some tweaking on the pre-existing points. I can't judge just from looking at the text how the adaptations would work, but they look ok and in line with the advertised purpose.

I ran the curves through the float curve editing and display tool to check they worked all right, and a couple of existing high-pressure curves looked quite weird (as in, some of them decreased below zero or decreased, increased and decreased again), so I edited their points a bit to make them obey logic more closely. You shouldn't notice much change ingame outside thick atmospheres.

Edited November 9, 2017 by voicey99

[medikohl]

Any plans to add another variable wing? Possibly a small/medium/large swing wing?

 

[linuxgurugamer]

33 minutes ago, medikohl said:

Any plans to add another variable wing? Possibly a small/medium/large swing wing?

 

Only if some else makes it.

[swjr-swis]

On 11/9/2017 at 1:46 PM, linuxgurugamer said:

I'd appreciate some other eyes to take a look at this

Ok, took me quite a bit longer than expected... github and my browser got me twice - github by 'faking' the latter half of diffs and my browser by crashing on me at one point and at reload resetting the page so I lost track of where I was, which made me have to go through all of it again to be sure. Nevertheless, here's my findings of the review:

TL;DR - as far as I can tell you can merge the PR wholesale without detrimental effect. The atmosphereCurve changes could use in-game testing/confirmation, but on text at least they look ok - and if anyone finds they need further refinement, it's easy enough to enter a new issue/PR for that later.

 

The vast majority of the changes have no effect on file functionality:

• Corrections to indentation - a big thank you for that, as it definitely helps in making the files more readable and to better visually group NODE{} content.
• Whitespace removal - A lot of blank lines removed; also very welcome, but as said before, a bit overdone to my personal taste - I prefer to see a line of whitespace between NODES{}. I think this was already corrected in a newer PR.
• Line/section order - some lines/sections moved up or down in the files in the interest of consistency, all good work.
• Cleanup/removal of superfluous/spurious comment text - Most of these seemed to be remnants from templates or copy/paste actions, good riddance.
• Spelling and grammar changes to part description texts - In most cases corrections of clear errors, and a few removals of spurious cut/paste remnants, which is all good. (#1)

A few changes do affect functionality, for the better:

• A section of MM patch text was cut out of a part cfg file (MEMDescentMod) and put into a new independent patch file (SXT_KIS.cfg), as it should have been
• A number of atmosphereCurves were adjusted/extended into the higher pressure range. The changes look ok at first glance, but they'd require testing in-game to be sure.
• The Oscar-C tank fuel content was upped a bit, and cost/mass changed to match stock ratios.
• One correction to a faulty tags line (tags = tags = ).
• One correction to a misformed bulkheadProfiles line.

 

A few remarks on the PR, mainly for @voicey99 to keep in mind in the future:

• In the future, try to split PRs to make the job of reviewing it manageable. For a mod creators/maintainer it is almost more time consuming to have to review someone else's code than to write their own, especially if it's done in huge chunks like this that don't allow for piece-meal processing. Also, it would've helped the logical processing of the different types of changes if they had been separate PRs.
• #1: A number of changes to description texts seem to be more based on personal preference than necessary corrections. In a few cases I think to have spotted a change based purely on a misinterpretation of the original intention of the description. Personally, where it is not a clear case of correcting a grammar/spelling mistake, I would opt to leave the original text of the mod author alone, but since none of it affects the functionality and it would expedite things for lgg, I'm not going to go into those details now. (If they bother me enough later, I may just enter a github issue with my suggestions)
• All that said: thank you for the time and effort in helping with PRs. Any help lgg gets in maintaining his ever-growing list of mods will help in keeping his effort viable, and the mods available.

[linuxgurugamer]

I'll get it merged and released tomorrow evening

[swjr-swis]

On 11/9/2017 at 7:53 PM, voicey99 said:

It's in a PR to the new branch.

I reviewed #37 on github. One of the descriptions you change in there needs a slight correction (see the review for the detail), other than that it looks good to go.

[voicey99]

9 hours ago, swjr-swis said:

I reviewed #37 on github. One of the descriptions you change in there needs a slight correction (see the review for the detail), other than that it looks good to go.

Is it the truck axle one missing a full stop?

11 hours ago, swjr-swis said:

A few remarks on the PR, mainly for @voicey99 to keep in mind in the future:

• In the future, try to split PRs to make the job of reviewing it manageable. For a mod creators/maintainer it is almost more time consuming to have to review someone else's code than to write their own, especially if it's done in huge chunks like this that don't allow for piece-meal processing. Also, it would've helped the logical processing of the different types of changes if they had been separate PRs.
• #1: A number of changes to description texts seem to be more based on personal preference than necessary corrections. In a few cases I think to have spotted a change based purely on a misinterpretation of the original intention of the description. Personally, where it is not a clear case of correcting a grammar/spelling mistake, I would opt to leave the original text of the mod author alone, but since none of it affects the functionality and it would expedite things for lgg, I'm not going to go into those details now. (If they bother me enough later, I may just enter a github issue with my suggestions)
• All that said: thank you for the time and effort in helping with PRs. Any help lgg gets in maintaining his ever-growing list of mods will help in keeping his effort viable, and the mods available.

I'm not sure how to break PRs like this up. If I split it into, say, four batches of 47 files, it's still the same amount of total reviewing work to do.

What sections of the description text did I get wrong? I changed them so they would read better grammatically, even if it wasn't strictly necessary. The only one I substantially changed was the TRV-500XXL, and that was because it was a CC of the 1600XXL (it even named the 1600 in it) and didn't have one of its own at all (so I wrote one).

Edited November 11, 2017 by voicey99

[swjr-swis]

52 minutes ago, voicey99 said:

Is it the truck axle one missing a full stop?

Yes, missing a full stop and a 'we': "The TK-W1 boasts a wider wheelbase since we kept flipping the M1s. Just add wheels."

 

54 minutes ago, voicey99 said:

I'm not sure how to break PRs like this up. If I split it into, say, four batches of 47 files, it's still the same amount of total reviewing work to do.

Still the same amount of total work, but:

• By simply splitting it up, there is less chance of things interrupting an individual PR review and perhaps forcing one to start all over again. Less time one has to dedicate in one chunk, which means one can better fit it into the RL schedule. Remember, not everyone has the same stretches of free time to dedicate to modding, sometimes especially mod authors/maintainers.
• But more importantly, I also said: "Also, it would've helped the logical processing of the different types of changes if they had been separate PRs." By doing functional changes in separate PRs from the whitespace changes, for example, a reviewer can work much more efficiently - it is quite faster to scroll over large stretches of code when you can be sure that you only have to look for empty lines added/removed and opening and closing brackets to be in the expected places. On the other hand, if you have to be on the lookout for code changes that affect functionality as well, the process becomes a lot slower. Also, a mod author can prioritize the PRs and like in this case, could've quickly reviewed and merged the atmosphereCurve/Oscar-C changes, which are arguably more 'bang for the buck', and leave the whitespace cleanups until there is more time/help.

(don't misunderstand me, I think most coders appreciate clean, consistent whitespace, but when one has limited time, function tends to override OCD. If it works, it works... we'll make it pretty when there's time).

[DeadMazay2000]

Very nice mod, but Agamemnon Nuclear Ramjet Engine doesn't work...  I hope you'll fix it)))

[linuxgurugamer]

1 hour ago, DeadMazay2000 said:

Very nice mod, but Agamemnon Nuclear Ramjet Engine doesn't work...  I hope you'll fix it)))

Define "doesn't work", that's rather nebulous 

Log file would be helpful

[linuxgurugamer]

@voicey99 Are all changes completed?