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

Rescale isometric world coordinates to measure 1024 along the cell axes. #13253

Merged
merged 6 commits into from May 9, 2017

Conversation

Projects
None yet
4 participants
@pchote
Member

pchote commented May 6, 2017

On current bleed ranges are measured horizontally on the screen, so a 2c512 range gives a circle intersecting with the corners of the cells to the left/right.

screen shot 2017-05-06 at 15 05 11

This is counterintuitive because most people expect ranges to be measured down the cell axis, meaning that the above should be closer to 3c512.

This PR rescales the World ⇆ Cell coordinate transformations for RectangularIsometric mods down by a factor of sqrt(2) so that the size running down the world axes is 1024 instead.

A 2c512 range (5 cell diameter) now matches intuition:
screen shot 2017-05-06 at 17 05 03

This issue has been known for a long time (but I can't find the issue # for it), but has been blocked on a combination of lack of interest, ugly upstream code that hardcoded transforms, and downstream breakage. Most of our ugly code has now been rewritten, and downstream breakage is mitigated by decoupling mod releases from specific engine versions.

I have included upgrade rules for the LocalOffsets, but have left speeds, ranges, etc untouched because these are already correct (were previously wrong) for the TS mod. Third party mods will need to rescale these values if they want to keep their current balance statistics.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote May 6, 2017

Member

Fixed.

Member

pchote commented May 6, 2017

Fixed.

@GraionDilach

This comment has been minimized.

Show comment
Hide comment
@GraionDilach

GraionDilach May 7, 2017

Contributor

I don't believe this was ever filed as an actual issue.

Contributor

GraionDilach commented May 7, 2017

I don't believe this was ever filed as an actual issue.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr May 7, 2017

Contributor

Needs rebase.

I don't believe this was ever filed as an actual issue.

It's possible that we only discussed it on IRC, yes (tbh, I don't even remember participating in such a discussion, but my memory is bad in general so that doesn't have to mean much).

Contributor

reaperrr commented May 7, 2017

Needs rebase.

I don't believe this was ever filed as an actual issue.

It's possible that we only discussed it on IRC, yes (tbh, I don't even remember participating in such a discussion, but my memory is bad in general so that doesn't have to mean much).

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr May 7, 2017

Contributor

👍 after the following yaml values have been adapted as well:

  • harvester's WithHarvestOverlay.Offset
  • Grenade's Speed (160 was optimized for old scaling, now it's too slow compared to original)
  • change ^DefaultMissile values like this to approximately restore the old flight curve:
		VerticalRateOfTurn: 11
		CruiseAltitude: 2172

I'll deal with anything else we might have overlooked, and some fine-tuning if necessary, in a dedicated PR.

Contributor

reaperrr commented May 7, 2017

👍 after the following yaml values have been adapted as well:

  • harvester's WithHarvestOverlay.Offset
  • Grenade's Speed (160 was optimized for old scaling, now it's too slow compared to original)
  • change ^DefaultMissile values like this to approximately restore the old flight curve:
		VerticalRateOfTurn: 11
		CruiseAltitude: 2172

I'll deal with anything else we might have overlooked, and some fine-tuning if necessary, in a dedicated PR.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote May 7, 2017

Member

Updated, but you should confirm your 👍 for the WithHarvesterOffset change.

Member

pchote commented May 7, 2017

Updated, but you should confirm your 👍 for the WithHarvesterOffset change.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr May 8, 2017

Contributor

@OpenRA/engine-hackers This is a dependency for the 3 referenced PRs plus one (or more) fine-tuning PR(s) and the upcoming TS hit-shape PR, so it would be nice to get this merged quickly.

Contributor

reaperrr commented May 8, 2017

@OpenRA/engine-hackers This is a dependency for the 3 referenced PRs plus one (or more) fine-tuning PR(s) and the upcoming TS hit-shape PR, so it would be nice to get this merged quickly.

@GraionDilach

👍

@atlimit8 atlimit8 merged commit 4a80c37 into OpenRA:bleed May 9, 2017

2 checks passed

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

This comment has been minimized.

Show comment
Hide comment
@atlimit8
Member

atlimit8 commented May 9, 2017

@pchote pchote deleted the pchote:rescale-isometric-world-coords branch Oct 15, 2017

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