...PART:DECOUPLER does not detect docking ports #2864
Comments
|
Tested after undocking and redocking and the problem still exists. So is not just with PREATTACHED ports. |
|
I agree that there is a bug here, but one of the primary assertions you have made is not correct. part:decoupler will return a staged dockingport. As an experiment, I build the same craft four times (with a different part 2) to see how it behaves.
Explaining the four values. I've been especially explicit in what I tested and the results I got, because from what I can see, in both the code and experimentally, your first two paragraphs are wrong. The part tree will always wrap a docking node in a DockingPortValue, which is a subclass of DecouplerValue, which will always be IsType("decoupler"). Bug 1. IsType("Decoupler") is inconsistent. Bug 2. Element:Decouplers is inconsistent. This will leave us with all four situations responding true to isType("decoupler"), which I believe is correct, but only the staged versions being in the lists of decouplers, or returned as a part:decoupler. |
|
A huge assumption on your part is that the bug will look exactly the same across different versions and crafts. I most certainly get repeatable results with staged docking ports not being reflected in CORE:PART:DECOUPLER |
|
I would also assert that in a kOS context, whether a separator or dock port is staged or not should not be relevant with regards to whether it is returned by PART:DECOUPLER because kOS can trigger a separation without the part being staged. In the craft pictured, I do not want the a launch sequence using the STAGE command to ever separate the two elements, but I most certainly want to perform automated tasks based upon detection of the elements separating. I can put the staged decoupler at the top of the staging sequence to live with the situation, but it is important to not force kOS into the same contexts as non-automated game play in general in my view and would prefer that staged or not, docking ports that separate potential or actual elements be reflected in PART:DECOUPER for separation detection purposes within scripts. In other words, PART:DECOUPLER should reflect potential and actual ELEMENT boundaries, not merely being in the non-automated game-play launch oriented staging sequence |
|
Hm, I do think docking ports and decouplers should always be returned by the :decoupler suffix regardless of whether they are staged. But what about heat shields? Their jettison action essentially turns it into a separator, and you might want to change the behavior based on whether the heat shield is staged or not. But maybe that's a conversation for a different day. |
|
Good question. In what use case does jettisoning a heat shield create another element? That would be my litmus test. If it is only separating itself the it isn't really a separator in the sense of separating different assemblies of the craft into elements. |
|
I don't know if you are also checking this but a docking port can only serve as a decupler when it is attached in the VAB once you decupple that node it will stay in the staging list but activating it will do nothing. |
|
@mgalyean If we assume that core:part is the root part (or near the root part), then the core:part:decoupler must always be null because there is no previous part (or decoupler) in the tree when it walks it. Presently, the :decoupler suffix tries to identify which part will detach this part from root through normal staging. Inspect a part on the other side of the decoupler, or change your root part. It makes the decoupler suffix less useful than might be assumed. We could choose to try harder and allocate a decoupler for those parts that don't have one, but this might require a more intuitive understanding of the rocket. Take a rocket that looks like a delta IV heavy - you can make it work mostly as you would like by having the root part in stage 2, but that was probably only advanced users (putting more than one cpu on a craft) that would choose that deliberately. @JonnyOThan I actually agree with you about having them all respond to the decoupler suffix, and I'd extend that to the decouplers lists as well. My hesitation is that its more of a compat buster - although probably a worthwhile one. A heat shield has a ModuleDecouple so as it stands, it will be a in the decoupler/decouplers suffix in the same circumstances as the others (yes if it is staged, no if it isn't). If we extend the decoupler suffix and decouplers lists to include unstaged decouplers and docking ports, I think you're saying that we don't include those with a ModuleJettison and I'd agree. @mgalyean I didn't test too hard, but for this discussion I think heat shields are just decouplers, and you can use heat shields as your decoupler if you like having more and variable mass. Instead of attaching a payload with a TD-06 or a clampotron jnr, use a 0.625m heat shield. @nuggreat As far as I've seen there is no check to see if a docking port has been staged, but makes sense. |
|
@BaconLauncher wrote "If we assume that core:part is the root part (or near the root part), then the core:part:decoupler must always be null because there is no previous part (or decoupler) in the tree when it walks it. " That is a fairly incredible assumption. I use CORE:PART:DECOUPLER all the time to detect element separation. It is nice that you agree with Jonny about them all responding to DECOUPLER as that is what I also mentioned. I kind of get the sense of getting wrapped around the axle of corner cases here. Heat shields as decouplers when they nearly always weigh more? Well, ok. But in that case they are separating an element in which case I think they should respond to PART:DECOUPLER. For me it would be simple: if it separates an ELEMENT or potential ELEMENT (see recent issue about preattached dock ports and elements) then it should respond to PART:DECOUPLER Maybe this is where we are getting off track: I put cores on my boosters to steer them back to KSC. I put cores on every ELEMENT, often more than one. |
|
@nuggreat , I've found that attached or docked docking ports out of the VAB, staging enabled or not, will always show up in the on-screen staging list. Maybe there is something wrong with my install. I haven't looked to see if they magically appear there when docking craft but I doubt it. Will look |
|
I have never seen docking ports on the staging list unless I enable staging on them (which I never do because they never leave the list even on full game restart), this could be an OS/mod thing if you are on something other than windows 10 and a fairly stock install. |
|
I'm running the latest 1.11.1 + BG so who knows. Not a big deal considering everything else |
|
@mgalyean I also use lots of cores, so I agree that its an incredible assumption - just pointing out that a core near root won't get a decoupler. I've used two different strategies over the years to solve this problem in other ways - walk the part tree looking for parts with a particular module, or make use of name tags so that the relevant decoupler/port is marked per core in the vab. Working out how to make these suffixes work is actually a new idea for me. On my stock/bg/mh install I can make docking ports appear and disappear from the staging list just by toggling staging. However they will only stage if that is done without changing their status. Once you have toggled them you can only manually activate. If they stage, they don't leave the staging list unless you disable staging on them. Sounds like a ksp bug. edit: actually following along from the above on enabling staging in flight, you can do this provided that you move it in the staging order before staging. You can then disable it, enable it, move it, and stage it again. |
|
Ok, that makes a lot more sense. Obviously the core in the actual final stage of my craft returns "None" for CORE:PART:DECOUPLER as one would hope. I think the rule is that if the core is in the same element as the craft ROOTPART, then it will have no CORE:PART:DECOUPLER the same as any other part in the same element as the craft ROOTPART. Make sense |
|
As for the heat shields, i'll go with the flow, but i'm going to have to put a check in my code so I don't accidentally consider a heatshield the part to watch for an element decoupling situation as I will never use it for such. FWIW, I found a faster way for an element's core to detect that its element has been decoupled than waiting for :DECOUPLER = "None". I've switched to testing for :DECOUPLER:ISTYPE("String") to become TRUE and it is typically much faster. I was getting strange long delays waiting for :DECOUPLER = "None", but the type changes happens very fast relatively speaking. Please don't change that, ha ha. Too much code depending on it now. Well, if you do, let me know. If there is any practical way to speed up decoupling detection from kOS then I'm all for it. I've had SRBs separate and nearly make it halfway to the ground before my scripts on them would move past waiting for DECOUPLER = "None" Ideally, I'd be able to register a listener for a decouple event instead of trying to find the right polling voodoo |
|
That delay is odd, because both require an pass over the entire ship to build the parts list. Which relates back to the methods mentioned above and their inconsistencies. As there are other ways that more advanced kos users can find what they want more precisely, these methods need to be as useful as they can be for those taking the more simpler approach, looking for rapid functionality in the 90% situation. Once you get into writing kos that doesn't make assumptions about where the root part is, using the STAGE command, or even if you are on the active vessel, you stop using these methods. I guess where I struggle is, having used these commands to find a decoupler, what do you do with it? A decoupler has no additional methods, so all I can think of is choosing to STAGE or checking what STAGE it is in, check its type (which my PR would break), or watch to see it becomes a different vessel than another part. If you are going to step into its module and do actions or events, you could have found it that way too. So given that, does it make the methods more useful or less useful to include unstaged decouplers and docking nodes in the DECOUPLER and DECOUPLERS methods? As my PR is right now, you will find ones that have a chance of being activated by STAGE, and you will ignore ones that don't. This seems right to me. |
|
I rather don't care about what is "correct". I only care about a consistent definition. Then the code can be made to follow that definition. Which definition is desired? Does a part being :decoupler describe it's physical characteristics or its flight plan characteristics? If it describes its physical characteristics then the fact that it physically could decouple means it should be a decoupler, regardless of whether the plan (staging list) expects it to. If it describes its flight plan characteristics then the meaning of :decoupler has to change when a vessel docks and undocks and the stock game screws up the staging list like it always does. The heat shield issue brings up a question - does the heat shield let you detach it when it's not the bottom-most piece? If so, then yeah it's physically a decoupler even though it's never really staged that way. If it requires being the bottom-most piece then it's not much of a decoupler because it only can detach itself, not a part attached under it. I've never tested which way around it is because I can't think of a valid design in which a heat shield doesn't have a dedicated decoupler part right under it anyway since the point of the heat shield is to be able to expose it as the bottom-most part at some point in the flight plan. |
|
I think you are more precisely stating my concern - the current methods seem to be flight plan methods. The heat shield to my testing acts just like a decoupler, and based on your other comments the flight plan should attempt to not include them in the DECOUPLERS because they are unlikely to be used as that. |
|
I really don't like the methods being flight plan methods though, because the naming implies they are answering the purely physical question "is there a decoupler part there?" and the part is still a decoupler whether it's in the staging list or not. But I guess I could be overruled on that. I think it would be ideal to always answer "yes it is", but then let you query separately to ask "okay, but is the decoupler staged or not?". I do agree the inconsistency between the two means of querying decouplers (element decouplers vs ship decouplers) is bad. It needs to go away and they need to both be working from the same definition as each other. |
|
Well the original inconsistency is fixed in the current PR. We could just add in the idea of listing or tagging all parts instead of just Staged ones. I don't think there is any point unless later we add methods to Decoupler such as :ISDECOUPLED and :DECOUPLE, and whatever else we feel like doing to them, but needing to be respectful of docking ports also which already has :UNDOCK. Starting to sound like a useful solution. For ISTYPE purposes, a new decoupler subclass could take the "Separator" nomeclature and the superclass keeps the "Decoupler". A docking port will be a "DockingPort" and a "Decoupler", what we call a decoupler will be a "Separator" and a "Decoupler". |
|
Frankly, my nightmare is that after the dust settles the way for a core to find out if its element has decoupled/separated/undocked will have become more, rather than less, complicated, and possible less stable. I write this because I'm not certain that what I thought was the only possible purpose of PART:DECOUPLER; to detect separation of subassemblies/elements, is a view necessarily shared by others. Though I admit I'm not sure it isn't shared, but the weeds have gotten so tall I'm not sure. The worst case scenario for me is that PART:DECOUPLER will be made to work in some perfectly self-consistent and elegant way but would be useless for anything practical, like reliably detecting element separation/undocking. I have always read PART:DECOUPLER to not just be answering the use case of getting the part that will separate this element/subassembly but also providing a means of detecting such a separation/undocking has occurred and would hate to see that get elegantly excised without there being a better approach for the real world use case |
|
Recommenting on this one. The PR as it currently stands is the minimum that fixes the inconsistency. All the ideas for enhancements are possible and fairly simple but seem to be lacking a clear objective, direction, or are potential backwards compatibility issues. |
|
From a purely use case POV, and only one data point, my only interest in the DECOUPLER suffix was to be able to know which part would separate the subass'y the core was in and to additionally use the DECOUPLER suffix to detect when that separation happened. I have since changed all my code to rely on a change in SHIP:ROOTPART to detect separation because the delay in the change in status of DECOUPLER from a part to a string equal to "None" can vary from anywhere between 1s to effectively never (booster goes out of physics range with separation never being "detected" by way of DECOUPLER changing type and value). ROOTPART is nearly instantaneous so other than not being to be sure the separation actually occurred at the decoupler and not part of a RUD, I'm locally satisfied. But yeah, there are some outstanding things to ponder here. I just wanted to get the ROOTPART approach (suggested by JonnyOThan) jotted here for others who end up here for similar reasons to me opening the issue in the first place |
|
Sorry I let this linger for a while because for much of February my graphics card wasn't working and KSP was painful to run. Now I'm back to looking at this and trying to catch up on what's going on here. From reading the code, there appears to be a very large difference, quite significant, between When I read the code for
When I read the code for
Regardless of the inconsistency of whether the definition of "decoupler" should or should not include "is staged", these two suffixes cannot agree so long as the element is talking about "a subset of myself that breaks myself in two" and the part is talking about "something outside myself that cuts me from the root". This came about because I was trying to figure out why For them to really be consistent, the
I did notice that |
|
My suggestion is just forget that the two terms share most letters and their order. They are two different concepts. In a more verbose world, the first would be called something like "STAGING_DECOUPLER" and the second "CONTAINED_DECOUPLERS" (off the top of my head). I wouldn't consider the second category relevent to the current issue we've all be discussing, and I have to wonder how many ppl actually use it. It might be a good thing to create a synonym for that is more descriptive, then phase out DECOUPLERS over time? |
|
Okay, now I think I see what you mean when you say it fixes the inconsistency, @BaconLauncher . Before, I didn't agree that it did because I thought your intent was to make Change the intent to " (Side note: This is why I strongly disagree with the claim in modern programming that its bad to have comments that redundantly tell you the same thing the code does. That redundancy is essentially a form of parity check between intent and outcome, helping others detect the difference between "author wanted to do this weird thing" versus "author made a mistake here".) In any case, the kOS documentation for all of these suffixes is horrible and we need to make the definitions of these suffixes explicit. Because it wasn't documented clearly what was intended with regards to the staging list and these suffixes, I kept going in circles between "this is a bug" versus "no, this weirdness was intended". (For example, I thought the fact that |
|
I noticed the difference between this and elements when first looking at this, and noticed how different elements was, how much work it would be to line them up, and that there is a different issue tracking that - so I didn't attempt to address it in this one. |
While dockingport:ISTYPE("DECOUPLER") returns true for docking ports, a downtree part doing a PART:DECOUPLER does not see docking ports as their decouplers.
Whether staging is enabled on the dock port makes no difference, which is ok for most purposes as one may not care if it is staged or not, but one would think that especially in the case where the docking port was in the staging sequence that is would be logical for PART:DECOUPLER to recognize the port as the decoupler that will separate part from the main craft as it is fairly clear.
I'm thinking that whatever nearest uptree part for which ISSTYPE("DECOUPLER") is true, PART:DECOUPLER should return that part whether it is staged or not as staging is an entirely different consideration when scripting vs normal play.
Forgot a big point. This issue definitely happens when ports are in the PREATTATCHED state but I still need to test if it is true for craft that were redocked after the initial undocking. So this may just apply to ports in the PREATTACHED state
The text was updated successfully, but these errors were encountered: