linuxgurugamer / FTLDriveContinued

Other
4 stars 4 forks source link

Beta 2 major overhaul, added Analysis Report #15

Closed arekbulski closed 6 years ago

arekbulski commented 6 years ago

Readded the window with all beacons. Shown on video: https://youtu.be/g3gFE3WXJ40

arekbulski commented 6 years ago

Total EC = sum over all drives, not time. Previous code displayed same phrasing.

linuxgurugamer commented 6 years ago

Better, but your video, while very nice, highlighted a weak area, in that it needs KER, and you said "I have no idea how you would manage without it" if the user isn't using KER. I will not release it in this state. See below for the solution.

I would have done the window a little differently, but that's more personal preference, I prefer a list.

Anyway, the one thing missing is the ability to click on one of the panels in the window to select that beacon. That would make this totally usable in stock, and also eliminate the need to have another window open, or, if you want to look at it another way, to have a different window open.

Regarding the labels about EC:

Having the " /s " following the number says that the number is a per-second number. I don't have time to dig, but a quick test on the old mod shows that the Total EC Required IS a per-second value. The test showed that it would use 234/s, and was somewhat more than a second, and it used 365 EC.

Also, you still need to add the ability to disable drive modules, for the reasons I explained earlier. The point you miss is that if you have more drives than needed, they ALL will drain EC and possibly use too much. The ability to disable a drive would prevent this.

And did you fix the problem of being able to activate the drive while landed (it shouldn't be allowed)

arekbulski commented 6 years ago
  1. I want to add buttons in ANalysis window to select each target. This will solve KER dependency. I just dont know how to implement changing target yet. Working on it.
  2. There is a mild bug in EC consumption code. It should be draining 234/s but... 10% gets used to maintain spin, 90% used to increase genforce. Bottom line it takes 11sec not 10 to get full spin. Working on it.
  3. Disabling drives is not needed in this case. I can even record a video for you showing that it works without it. If you have too many drives then you spin for 1-2 seconds and jump. It consumes less EC in total over time. Working on it...
  4. Restrictions to be fixed, working on it.
linuxgurugamer commented 6 years ago

Disabling drives is not needed in this case. I can even record a video for you showing that it works without it. If you have too many drives then you spin for 1-2 seconds and jump. It consumes less EC in total over time. Working on it...

Would be fine if it automatically jumped, but you removed that, so if someone gets distracted, it can fully drain a ship over time.

linuxgurugamer commented 6 years ago

If you insist on using the targetedVessel', I think you can change it by changing this:

FlightGlobals.ActiveVessel.targetObject

linuxgurugamer commented 6 years ago

I want to add buttons in ANalysis window to select each target. This will solve KER dependency. I just dont know how to implement changing target yet. Working on it.

Just a suggestion, make each panel a complete button, so that all they would have to do is click on the panel, rather than a separate button

arekbulski commented 6 years ago

Check this out: https://youtu.be/ecB-ENwxy1g

All issues solved, looking into turning panels into buttons.

linuxgurugamer commented 6 years ago

Better, but....

  1. It is not obvious that the select button on each is for the one above. If you are going to leave the buttons, either put the button inside the panel, or to the right of the panel

  2. Either change the color or put some space after the first panel which shows the info in the current vessel

  3. If the continuing EC usage is for keeping the drive spinning, why is it that in the early part of the video, a single drive is using something like 2.3 EC/s, and later, when you have 3 drives, it's only using 0.5 EC/s?

  4. I still don't understand why you split the jump from the activate. It's an extra, unnecessary action.

arekbulski commented 6 years ago

1 and 2. Looking into it. Giveatime. :)

  1. Second vessel was on higher orbit, it needed less force to jump. Less needed force => less EC drain to accelerate spin => less EC to maintain spin. I just wanted to show you that on overpowered ships, EC drain is a function of needed force (not max genered force and not number of drives either).
  2. I dont have a justification... its part of my vision. Just take the cake for what it is. :)
linuxgurugamer commented 6 years ago

1 & 2, was just giving some feedback. 3, ok.

  1. I'll probably let it go for now, but I get complaints from users, I'll change it back
arekbulski commented 6 years ago

Errr... I hate to change my own vision for the mod, but I hate to leave players with something they dont like just as much. Can you share with me (4), some of their rants?

arekbulski commented 6 years ago

I have few improvements still on wishlist but you can accept the mod as it is right now. Here is what:

linuxgurugamer commented 6 years ago

it should have said "IF but I get complaints from users, I'll change it back" No rants yet, but I am anticipating.

linuxgurugamer commented 6 years ago

Editor mode requires you work with proto vessels. If you want to do that, it's ok, but I don't consider it a deal-breaker. If you don't, then don't show that value in editormode

arekbulski commented 6 years ago

Lets publish now. I discard the editor upgrade, had problems with it.

arekbulski commented 6 years ago

I should mention: patch affects CTT but doesnt stock tech. Didnt know how to implement it.

arekbulski commented 6 years ago

Refactored common config into single patch for maintainability.

linuxgurugamer commented 6 years ago

You have now forced a dependency which wasn't there before. This mod didn't require ModuleManager, now you are forcing it. It's 4 parts, and this isn't going to be changed anytime in the future.

Please revert that last commit, where you refactored the common config

linuxgurugamer commented 6 years ago

Regarding this:

I should mention: patch affects CTT but doesnt stock tech. Didnt know how to implement it.

The CTT patch? I'm not sure what you are referring to here

arekbulski commented 6 years ago

Ah sorry, I thought that MM was a given. You sure you want me to revert that one?

Yes, CTT patch affects only CTT tech req. Stock tech requirements are not implemented.

arekbulski commented 6 years ago

Note that all the CTT/RSS/TS patches also require MM.

linuxgurugamer commented 6 years ago

Yes, but those are optional. If you don't use CTT, and don't use RSS and don't use TS, you don't need MM

arekbulski commented 6 years ago

Okay dokies... If you ask me, MM is so common that you might as well assume that it is there. But I will back it up, one second.

linuxgurugamer commented 6 years ago

It may be, but I don't want to add a dependency for no real reason. Thank you. Once you've done that, I'll merge into dev and start testing

arekbulski commented 6 years ago

Reverted and pushed. Good to go.

linuxgurugamer commented 6 years ago

ok.

What do you mean by "Stock tech requirements are not implemented."

arekbulski commented 6 years ago

This file patches ONLY IF CTT is installed https://github.com/linuxgurugamer/FTLDriveContinued/blob/dev/GameData/FTLDriveContinued/Parts/CommunityTechTree_patch.cfg

otherwise techneeded is not changed (its still SpecializedControl in CFGs) https://github.com/linuxgurugamer/FTLDriveContinued/blob/dev/GameData/FTLDriveContinued/Parts/FTLDriveS2/FTLDriveLarge.cfg#L32

Here is what we agreed on forum https://forum.kerbalspaceprogram.com/index.php?/topic/150178-131-ftl-drive-continued/&page=5&tab=comments#comment-3231595

linuxgurugamer commented 6 years ago

First problem: I put a few beacons in orbit. I then put a ship on the pad with a drive. Target vessel was none or invalid, so far so good.

I then showed the destinations, and selected one of them, but the target vessel in the right-click menu didn't change Going to orbit, it worked. It just doesn't look good. I would rather the following be done:

  1. On land, target vessel should NOT be shown, or replaced with something like "FTL Jump not possible landed"

I remember what we agreed, but what I don't understand was your statement about the stock tech requirements. Do you mean that you didn't change the part cfgs? That's ok, I'll do that

arekbulski commented 6 years ago

"However, don't change the existing files, do this (or whatever you suggest regarding the tech trees) as a MM patch, so that the patch can be deleted if not wanted. Preferably as a PatchManager controlled patch"

You asked me to implement it as a patch, so even with CTT it would still require MM.

linuxgurugamer commented 6 years ago

Also, you didn't change the select button. Again, not a big deal, I know how to do that, have done it several times before

arekbulski commented 6 years ago

Ah now I get it. Yes, destinations window shows vessels with an active beacon, regardless of flight mode and force required.

linuxgurugamer commented 6 years ago

I just got the window working with just clicking on the panel, would you like the code? Not finished, but would show you how to do it

arekbulski commented 6 years ago

Paste a screenshot!

linuxgurugamer commented 6 years ago

https://imgur.com/a/hWuiJ

linuxgurugamer commented 6 years ago

Like I said, it was a quick proof-of-concept, but it works All it needs is some way to indicate the selected target

arekbulski commented 6 years ago

It doesnt say anywhere that you can click those panels, player would not know that.

linuxgurugamer commented 6 years ago

I said it was a proof of concept.

linuxgurugamer commented 6 years ago

I'll add what's needed to make it obvious, will take about 10 minutes

linuxgurugamer commented 6 years ago

Here it is: https://imgur.com/a/hWuiJ I added color to the window to make it obvious which was selected

linuxgurugamer commented 6 years ago

I have to go, but I pushed my changes up into a new branch, dev2

arekbulski commented 6 years ago

Looks very nice, let me try to improve it tomorrow, the destinations window.

linuxgurugamer commented 6 years ago

Great

linuxgurugamer commented 6 years ago

Do you think you can replace all those if/then/else for the status strings with a simple case statement?

linuxgurugamer commented 6 years ago

I changed the if/then/elses into a case statement and pushed it to the dev2 branch.