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

Vectorize starting ship wings #5648

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

JohnAFernandez
Copy link
Contributor

Very Drafty, needs testing, but it's a good next step for getting starting wing to be basically dynamic, and a step towards removing the retail ui limits, which is a further step towards removing the MAX_SHIP_CLASSES limit.

wingp = &Wings[Starting_wings[i]];

ai_maybe_add_form_goal( wingp );
// Cyborg - And we should be dealing with TVT wings properly, as well.
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually you can just look at Starting_wings and not bother with TVT_wings. This bit of code syncs up the two lists immediately after the mission is parsed:

	// when TVT, hack starting wings to be team wings
	if (MULTI_TEAM)
	{
		Assert(MAX_TVT_WINGS <= MAX_STARTING_WINGS);
		for (int i = 0; i < MAX_STARTING_WINGS; i++)
		{
			if (i < MAX_TVT_WINGS)
				Starting_wings[i] = TVT_wings[i];
			else
				Starting_wings[i] = -1;
		}
	}

So in most cases the MULTI_TEAM check is not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like relying on that. The point is to make fewer hacks and less code that relies on those hacks. Otherwise I make more work for someone else later.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not a hack, despite what the comment says. It's taking a specific starting configuration and making it work in a standard way. It simplifies a lot of code, and it removes the requirement to use the if (MULTI_TEAM) check every time you access it. In fact a few weeks ago I fixed a couple bugs in the code caused by not-quite-correct MULTI_TEAM checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Except that having it this way makes it more difficult to expand the number of wings in TVT. We would need specialized code just to work around them sharing data like this, which would be bug prone. Right now it's implicit behavior that is not immediately obvious, unless you happen to run across this specific block.

In any case, I don't think discussing this further is helpful. I have spent a lot of time working on this, and I have many more hours to go, and arguing about whether or not this is hackish behavior does not advance us any further.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not sharing data, it's literally reconciling the configuration between the parsing code and the mission code. I've been over the mission parsing with a fine-toothed comb. This section is appropriate, it's needed, it prevents bugs, and I've fixed bugs caused by the pattern not being followed.

This is not arguing over whether something is or isn't a hack, this is describing an important design feature of the parser. I've spent a lot of time working on this as well. My reasons for pointing this out are several: a) I don't want my fixes to turn back into bugs; b) I don't think a complicated pattern should be used when a simple pattern will do; and c) I want to share what I've learned so that you don't spend hours learning what it took me hours to learn. (That's why I mentioned this now rather than waiting until it's out of draft.)

We can schedule a code deep dive over DM if that would be better than hashing it out on Github.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think discussing this further is helpful.

This is an essential first step for fixing the loadouts UI.  By removing the need for a Fixed number of starting wings, in mission files, that the UI can likewise break those assumptions without too much issue.

Also, all possible MAX_STARTING_WINGS references removed
@wookieejedi wookieejedi added the cleanup A modification or rewrite of code to make it more understandable or easier to maintain. label Oct 2, 2023
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants