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

remove the special_ship field #4889

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Goober5000
Copy link
Contributor

This field existed in retail, but was not used for its intended purpose. Subsequently, I modified the functionality so that it worked as advertised, but this has side-effects for retail missions which specify a special ship other than the wing leader. I'm not sure to what extent this field actually enhances a mission, so I propose removing it.

@Goober5000 Goober5000 added cleanup A modification or rewrite of code to make it more understandable or easier to maintain. discussion This issue has (or wants) a discussion labels Nov 12, 2022
@JohnAFernandez
Copy link
Contributor

On board, as long as we still put the field in new mission files for backwards compatibility. There's no reason to introduce a compatibility break.

@Goober5000
Copy link
Contributor Author

The field would not be saved to new missions, but it would still be recognized on mission load, and it would be saved to missions saved in retail format. This is similar to other modified fields such as the migration of +Orders Accepted to +Orders Accepted List.

@EatThePath
Copy link
Contributor

this seems like a risky change to me. Even if it doesn't do what it's documented as doing it sounds and looks in the code like it does something? Regardless of if that something seems useless, I'd be a concerned with subtle mission rot for old missions that may have intentionally or unintentionally used that behavior.

@Goober5000
Copy link
Contributor Author

It's not quite that simple. The field does perform as advertised now. There are a few specific problems. First, some retail missions had spuriously-set "special ship" fields which means those retail missions now behave differently. Second, wing formations and waypoint code assume that the first ship is the leader, simply based on geometry. And that's aside from the fact that for both story and gameplay purposes, wing leaders are expected to be the first ship in the wing.

@Goober5000
Copy link
Contributor Author

Goober5000 commented Nov 13, 2022

As a matter of fact, looking through the patch, the only meaningful, consequential use of special_ship is in the form-on-wing goal, and that conflicts with the wing formation code. The other uses of special_ship are for choosing which ship in the wing is used for bookkeeping.

@Goober5000
Copy link
Contributor Author

I'll need to update this after the latest changes.

@Goober5000
Copy link
Contributor Author

Updated.

@Goober5000 Goober5000 force-pushed the remove_special_ship branch 2 times, most recently from a3aa038 to 8f63cd8 Compare November 28, 2022 17:04
@JohnAFernandez JohnAFernandez added the Waiting for Stable Marks a pull request that is to be merged after the next stable release, due to a release cycle label Dec 19, 2022
@BMagnu BMagnu removed the Waiting for Stable Marks a pull request that is to be merged after the next stable release, due to a release cycle label Feb 14, 2023
Copy link
Contributor

@EatThePath EatThePath left a comment

Choose a reason for hiding this comment

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

I believe this does create unacceptable compatibility breakage. It is deep corner cases that quite possibly don't come up in any mission that has ever been made, but neither can we be sure of that. From reading the changes again, it seems clear that certain things do depend on what ship is the leader, such as some details of what orders the wing will respond to an how. Wings are rarely but sometimes composed of more than one ship class, so assuming ship 0 is the wing leader/special ship is not a safe change to make. Moreover, if any breakages do occur they would be subtle ones that are hard for users to notice. I don't think the streamlining is worth the potential for modrot, if nothing else.

I'll note that I haven't dug through the history to read all the changes mentioned above and compare the post-change functionality to before any of the recent fixes discussed there, so I'm basing this currently off the delta in this PR alone. I will strive to do that further reading in the near future and adjust accordingly, or reconsider my objection if another reviewer does so and finds there's no actual loss here. But even if it's simply a reversion to the status quo I also feel that removing the field from file writing is, much like the recent changes that spurred the mission file versioning changes, at best negligible benefit and minor modder inconvenience. I do not like that balance at all, regardless of how many times in the past it has been deemed acceptable.

@Goober5000
Copy link
Contributor Author

Finally surfacing through my to-do list stack to respond:

I looked through the entire diff just now, and it's been several months so I was able to look at it with fresh eyes. And I can confirm now what I said then: the field is almost entirely vestigial. Look through the code, and look at the consequences of each of the code sections. I'll be happy to walk you through them if you want.

  1. There are two places where the special ship is related to the orders the wing will accept. The first only checks that the ship is a fighter or bomber; you're not going to have a situation where part of the wing is a fighter/bomber and part of the wing is something else, especially in a squad messaging situation. The second merely compares the special ship against every other ship; that will have the same effect whether the special ship is 0 or some other number.
  2. As I mentioned earlier, the one non-vestigial change is related to formation flying, but the formation code assumes elsewhere that the first ship is the focus for the formation. This is likely to have no practical effect.
  3. Removing vestigial fields from the codebase is good practice: it makes the code cleaner, makes it easier to read, reduces maintenance cost, and very slightly optimizes it.
  4. Removing vestigial fields from FRED keeps the format tidy and avoids implying that the field is used when it really isn't. I respect the "don't change the mission file" argument, but a) we have mission versioning to accommodate that now; and b) the format has already changed during this release cycle.

This field existed in retail, but was not used for its intended purpose.  Subsequently, I modified the functionality so that it worked as advertised, but this has side-effects for retail missions which specify a special ship other than the wing leader.  I'm not sure to what extent this field actually enhances a mission, so I propose removing it.
@EatThePath
Copy link
Contributor

EatThePath commented Dec 8, 2023

I'm afraid I still have to conclude this is dangerous. If nothing else is, the killers are eval_object_ship_wing_point_team and object_ship_wing_point_team::object_ship_wing_point_team(wing* wp). That uses the special ship field to determine what ship to reference for anything that references a wing via OSWPT, from what I can tell, which makes it a massive field of possible subtle behavior changes. This doesn't seem like just bookkeeping to me, that could mean significantly different sexp effects or return values outcomes depending on what ship is used. Non-0 special ship values do exist in the wild, and so do heterogeneous wings. Yes, they're rare, and yes if there's reinforcement waves involved it muddies the waters, but I don't think that clears the issue. And the OSWPT usage could mean side effects even for homogeneous wings, in my mind.

Side effects for this may be tiny and they may be rare, but that to me just means they're prime for the most insidious type of mod rot that we should be striving to avoid. It also means there could be cool niche uses of that functionality we haven't thought of, which makes me against removing it without a compelling reason.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup A modification or rewrite of code to make it more understandable or easier to maintain. discussion This issue has (or wants) a discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants