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

Fix Contrail solid delay of 4 &color fix #19089

Merged
merged 1 commit into from
Dec 16, 2021
Merged

Conversation

dnqbob
Copy link
Contributor

@dnqbob dnqbob commented Jan 26, 2021

fix #19084

Make contrail work for both projectile and aircraft with smoothest behavior without solid delays reduce solid delay to 1.

And make color fade away with only alpha, not becoming white first.

@dnqbob dnqbob marked this pull request as draft January 26, 2021 00:23
@dnqbob
Copy link
Contributor Author

dnqbob commented Jan 26, 2021

Turned to draft, I should still smooth from tail direction

@dnqbob dnqbob marked this pull request as ready for review January 26, 2021 02:01
@dnqbob
Copy link
Contributor Author

dnqbob commented Jan 26, 2021

OK, it is cool now

@dnqbob
Copy link
Contributor Author

dnqbob commented Jan 26, 2021

@reaperrr

@reaperrr
Copy link
Contributor

Contrails are still invisible when renderLength == 1 (kind of makes sense, as Index will always return the same WPos if there's only one in the array, and i < renderLength will never be true either in that case).
Not supporting a length of 1 feels wrong though (from a modder perspective), so I'd suggest something like this to support it:
length1-suggestion

There might be a more elegant solution, this is just the best I could come up with.

@pchote
Copy link
Member

pchote commented Jan 26, 2021

Not supporting a length of 1 feels wrong though (from a modder perspective), so I'd suggest something like this to support it:

That sounds like a naming/definition issue: you need two points to define a line, so the shortest option exposed to the modder as length 1 should be when there is two points.

@dnqbob
Copy link
Contributor Author

dnqbob commented Jan 26, 2021

Not supporting a length of 1 feels wrong though (from a modder perspective), so I'd suggest something like this to support it:
length1-suggestion

There might be a more elegant solution, this is just the best I could come up with.

Good point, but I think probably we can put it later Fixed in second commit but I think probably we can put it later

  1. I really need someone relatively fully check/test the this fix. I don't know if this version has some wrong/strange rendering at certain condition, and if this fix is proper for missile projectile. (While I was fixing&understanding this part, I got many strange rendering results from color to contrail position, don't know if there is still any)

  2. Don't want to make my fix here too big, and that makes test time and merge time longer.

update:
3. tried to solve and make it point instead of segment based but failed, and it is more complicated than I think, so plz put it later, maybe we can have better idea

@dnqbob
Copy link
Contributor Author

dnqbob commented Jan 26, 2021

And probably we should directly add 1 to all length when contrail is created after this PR?

Now as you can see if renderlength == 2, the Render() will only renders 1 length, which is also buggy.

@dnqbob dnqbob force-pushed the contrail-fix branch 2 times, most recently from e9847bd to c45e972 Compare January 27, 2021 11:05
@dnqbob dnqbob marked this pull request as draft January 27, 2021 11:10
@dnqbob dnqbob marked this pull request as ready for review January 27, 2021 12:58
@dnqbob
Copy link
Contributor Author

dnqbob commented Jan 28, 2021

@pchote I suddenly worry that, will .Net executes the

(renderLength - i) * 1000 / renderLength * color.A / 1000

by the order from left to right and don't optimize *1000 and /1000?

@abcdefg30
Copy link
Member

Those likely shouldn't be optimized to avoid big rounding errors?

@dnqbob
Copy link
Contributor Author

dnqbob commented Jan 28, 2021

Those likely shouldn't be optimized to avoid big rounding errors?

yes. although it performs like it won't optimize it.

Update:
use << 10 to solve this bug and increase performance.

@dnqbob
Copy link
Contributor Author

dnqbob commented Feb 18, 2021

And we can now use this contrail as high speed bullet line
F

@dnqbob
Copy link
Contributor Author

dnqbob commented Mar 28, 2021

OK since we worry about storage and time cost so I use a simple and stupid way.

@dnqbob
Copy link
Contributor Author

dnqbob commented Mar 28, 2021

fix comment

OpenRA.Mods.Common/Graphics/ContrailRenderable.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Graphics/ContrailRenderable.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Graphics/ContrailRenderable.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Graphics/ContrailRenderable.cs Outdated Show resolved Hide resolved
@@ -73,11 +73,31 @@ public void Render(WorldRenderer wr)
// Start of the first line segment is the tail of the list - don't smooth it.
var curPos = trail[Index(next - skip - 1)];
var curColor = color;
for (var i = 0; i < length - skip - 4; i++)

for (var i = 1; i < renderLength; i++)
Copy link
Member

Choose a reason for hiding this comment

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

Why was this changed from 0 to 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because renderLength is now length - skip, for readable I make i = 1 instead of make i < renderLength - 1.

If we make i = 0 base on this design, the contrail will be rendered to (0,0) in map.

Copy link
Contributor Author

@dnqbob dnqbob Mar 28, 2021

Choose a reason for hiding this comment

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

This loop part was tricky and with low readability so I tested frequently to get the correct boundary.

@dnqbob
Copy link
Contributor Author

dnqbob commented Mar 29, 2021

So should I mark the conversation as solved or the reviewer?

@dnqbob dnqbob requested a review from pchote August 19, 2021 12:17
Mailaender
Mailaender previously approved these changes Dec 1, 2021
Copy link
Member

@Mailaender Mailaender left a comment

Choose a reason for hiding this comment

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

I can confirm that this does nice contrails with length 4.

@dnqbob
Copy link
Contributor Author

dnqbob commented Dec 2, 2021

I guess I also need a rebase

nextZ += prepos.Z;
}

nextPos = new WPos((int)(nextX / k), (int)(nextY / k), (int)(nextZ / k));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
nextPos = new WPos((int)(nextX / k), (int)(nextY / k), (int)(nextZ / k));
var nextPos = new WPos((int)(nextX / k), (int)(nextY / k), (int)(nextZ / k));

And then remove the var nextPos = WPos.Zero; line above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

// Need at least 4 points to smooth the contrail over
if (length - skip < 4)
var renderLength = length - skip;
if (renderLength <= 0)
Copy link
Member

Choose a reason for hiding this comment

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

I assume with a renderLength of 1 we will never be able to draw a line, so this should be:

Suggested change
if (renderLength <= 0)
if (renderLength <= 1)

?

Copy link
Contributor Author

@dnqbob dnqbob Dec 9, 2021

Choose a reason for hiding this comment

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

fixed, but we may need to comment it here about why it must >=1 (and I don't know how to explain on this)

@@ -63,8 +65,10 @@ public IRenderable OffsetBy(in WVec vec)
public IFinalizedRenderable PrepareRender(WorldRenderer wr) { return this; }
public void Render(WorldRenderer wr)
{
// Need at least 4 points to smooth the contrail over
if (length - skip < 4)
// Note: The length of contrail is now acutally the number of the points to draw the contrail
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor spelling nit: "actually"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@abcdefg30 abcdefg30 merged commit c968a2a into OpenRA:bleed Dec 16, 2021
@abcdefg30
Copy link
Member

Changelog

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Contrail change
6 participants