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 crash in SpreadDamageWarhead if range is >= 32c0 #13645

Merged
merged 1 commit into from Jul 15, 2017

Conversation

Projects
None yet
5 participants
@jrb0001
Contributor

jrb0001 commented Jul 14, 2017

No description provided.

@GraionDilach

This comment has been minimized.

Show comment
Hide comment
@GraionDilach

GraionDilach Jul 15, 2017

Contributor

This doesn't really sit well with me considering that IIRC LengthSquared was calculated as long internally for a good while.

EDIT: or not. Blame doesn't show changes in the timeframe I remembered.

Contributor

GraionDilach commented Jul 15, 2017

This doesn't really sit well with me considering that IIRC LengthSquared was calculated as long internally for a good while.

EDIT: or not. Blame doesn't show changes in the timeframe I remembered.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jul 15, 2017

Member

What are you using int2 for that hits this limitation?

Member

pchote commented Jul 15, 2017

What are you using int2 for that hits this limitation?

@jrb0001

This comment has been minimized.

Show comment
Hide comment
@jrb0001

jrb0001 Jul 15, 2017

Contributor

SpreadDamageWarhead crashes if the range is >=32c0 if I have calculated that correctly.

Contributor

jrb0001 commented Jul 15, 2017

SpreadDamageWarhead crashes if the range is >=32c0 if I have calculated that correctly.

@GraionDilach

This comment has been minimized.

Show comment
Hide comment
@GraionDilach

GraionDilach Jul 15, 2017

Contributor

@pchote For the record, he's using a SpreadDamageWarhead to affect the entire map and I find the idea horrible. We're already in a heated argument regarding this.

Contributor

GraionDilach commented Jul 15, 2017

@pchote For the record, he's using a SpreadDamageWarhead to affect the entire map and I find the idea horrible. We're already in a heated argument regarding this.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jul 15, 2017

Member

@jrb0001: SpreadDamageWarhead doesn't appear to use int2...? It uses WDist, which has already been fixed to use longs, so i'm a bit confused at this point.

Member

pchote commented Jul 15, 2017

@jrb0001: SpreadDamageWarhead doesn't appear to use int2...? It uses WDist, which has already been fixed to use longs, so i'm a bit confused at this point.

@jrb0001

This comment has been minimized.

Show comment
Hide comment
@jrb0001

jrb0001 Jul 15, 2017

Contributor

@pchote SpreadDamageWarhead uses RectangleHitshape which uses int2 iirc.

Contributor

jrb0001 commented Jul 15, 2017

@pchote SpreadDamageWarhead uses RectangleHitshape which uses int2 iirc.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jul 15, 2017

Member

The real issue in that case is that RectangleHitshape.DistanceFromEdge is using int2 to do calculations onWVecs. A more appropriate fix there to to make that use WVec and .HorizontalLength instead.

int2 is supposed to be used for screen-space pixel coordinates where overflow is not an issue.

Member

pchote commented Jul 15, 2017

The real issue in that case is that RectangleHitshape.DistanceFromEdge is using int2 to do calculations onWVecs. A more appropriate fix there to to make that use WVec and .HorizontalLength instead.

int2 is supposed to be used for screen-space pixel coordinates where overflow is not an issue.

@jrb0001 jrb0001 changed the title from Fix calculation of big Length in int2 to Fix crash in SpreadDamageWarhead if range is >= 32c0 Jul 15, 2017

@jrb0001

This comment has been minimized.

Show comment
Hide comment
@jrb0001

jrb0001 Jul 15, 2017

Contributor

@pchote is this what you meant? It works although I don't understand why you want to keep broken code which doesn't even have any comment that you should only use it in specific cases.

Contributor

jrb0001 commented Jul 15, 2017

@pchote is this what you meant? It works although I don't understand why you want to keep broken code which doesn't even have any comment that you should only use it in specific cases.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jul 15, 2017

Contributor

In all likelyness this will fix #11502.

Contributor

reaperrr commented Jul 15, 2017

In all likelyness this will fix #11502.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jul 15, 2017

Member

@pchote is this what you meant?

Yes, thanks.

I all likelyness this will fix #11502.

I concur.

Member

pchote commented Jul 15, 2017

@pchote is this what you meant?

Yes, thanks.

I all likelyness this will fix #11502.

I concur.

@GraionDilach

Looks good to me as well.

@abcdefg30 abcdefg30 merged commit 224ca78 into OpenRA:bleed Jul 15, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@abcdefg30 abcdefg30 referenced this pull request Jul 15, 2017

Closed

Crash @ nuke #11502

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30
Member

abcdefg30 commented Jul 15, 2017

@jrb0001 jrb0001 deleted the jrb0001:lengthfix branch Jul 15, 2017

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