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

Adding Rotation Logic to Renderer for use with Interpolated Facings #19889

Merged
merged 1 commit into from Jul 17, 2022

Conversation

AspectInteractive2
Copy link
Contributor

@AspectInteractive2 AspectInteractive2 commented Jan 19, 2022

This PR adds the ability for sprites to be rotated by the renderer, in order for additional interpolated facings to be created in between existing facings.

E.g.: 16 facings can be interpolated to 32 by adding an additional 16 interpolated facings. These interpolated facings are derived from the existing faces, by applying a fixed rotation that is proportionate to the gap between each facing.

Add Interpolated Facings
YAML flag: InterpolatedFacings
YAML options: Any number between 2 and 1024, that is greater than the number of Facings, and a power of 2.

Since sprites typically only have a set number of facings (e.g. 16 or 32), units that can rotate or move in more directions than they have facings will not have smooth movement between these facings. However, if we are able to rotate the sprite graphic between facings, we can create 'intermediate' facings between facings, as shown below. The code here takes the delta between the actual facing and the next intended interpolated facing (depending on how many are required), and applies this as a rotation to the sprite. The most common number of intended interpolated facings is 1024, as this is the maximum number of facings, however any number that is greater than the existing number of facings, that is also a power of 2 can be used as the InterpolatedFacings amount.

This rotation logic was used in the original red alert to smoothen the rotation of all aircraft with 16 facings. So to mimic the original game, I have added this flag to all aircraft that currently have 16 facings. As you can see in the below video, the movement is much smoother once this flag is enabled (right side of the video). By request I have also added this to the carry-all in D2K, in an attempt to smoothen jitters this unit appears to have in the current build.

https://www.dropbox.com/s/u7cucyvd3hjvti0/mig_orig_interp_compare.mp4?dl=0

A second use for this same feature is to use just 1 facing, and rely entirely on the InterpolatedFacings flag to create all remaining faces (e.g. 16, 32, or any number up to 1024). This enables sprites to rely entirely on the renderer's new rotation for their facings. While this is not as suitable for units that have perspective (since the shadows and lighting are not rotated, it will look a bit off), it is highly useful for flat or top-down viewed objects such as projectiles, or top-down units such as in the indie RTS Rusted Warfare (in fact this feature was my original motivation for this PR). This solves the following issue: #19146. This also greatly reduces the time required to develop such sprites, since only 1 facing (north) needs to be produced instead of 16 or 32. The below video shows what this feature may look like applied to the existing medium tank. While the tank here is not fully suited to the feature, it shows that the tank can appear and function almost like the original, despite only 1 facing being used (since all other facings are rotations).

https://www.dropbox.com/s/qsfa0fv7oe3dtp0/2tnk_userotationsforfacings.mkv?dl=0

Caveat
All of my tests have been in Red Alert and D2K. While the feature is not needed for other mods, I am unsure how it would interact with them, e.g. if there would be any unintended side effects. I don't think this is likely, but it is still possible.

Copy link
Member

@abcdefg30 abcdefg30 left a comment

Choose a reason for hiding this comment

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

Three questions:

  • Can we undo several of the line breaks?
  • Why did you declare all int parameters either in or out? Is that really necessary?
  • Can you please use tabs instead of spaces for the yaml indentation?

@AspectInteractive2
Copy link
Contributor Author

AspectInteractive2 commented Jan 19, 2022

Three questions:

  • Can we undo several of the line breaks?

So my rationale is that now that DrawSprite takes an additional parameter, it is not possible to fit the full method signature on one line without leading to scrolling. Now not all calls to DrawSprite use the additional parameter, but I thought it would be inconsistent to have some of the DrawSprite methods on two lines and others on one, so I opted to put all of them on two lines. Also this means if the layer or rotAngle parameters need to be added at a later time, no extra line breaks needed to be used. But if you need it, of course I can change it back for the DrawSprite calls that don't currently use the rotAngle parameter.

  • Why did you declare all int parameters either in or out? Is that really necessary?

The 'in' modifier was in part to match the existing parameters that DrawSprite uses, such as a,b,c,d,tint,size,alpha. But I think it is good practice, because this highlights that no downstream method can modify the upstream parameters, which is especially important for the renderer since the renderer should not alter logic upstream (I suspect this is why they were added).

The 'out' parameter has to be used in order for the DefaultSpriteSequence to be able to modify the Animation's rotation angle, while also returning the sprite. The alternative would be to create a struct with the Sprite and the rotAngle, but that would be messier and wouldn't provide much benefit apart from omitting the 'out'. Also this method signature was suggested by pchote initially, and I ended up finding it appropriate for the task.

  • Can you please use tabs instead of spaces for the yaml indentation?

I thought I did but happy to change this.
EDIT: This has now been fixed.

OpenRA.Game/Graphics/RgbaSpriteRenderer.cs Outdated Show resolved Hide resolved
OpenRA.Game/Graphics/SequenceProvider.cs Outdated Show resolved Hide resolved
OpenRA.Game/Graphics/SpriteRenderable.cs Outdated Show resolved Hide resolved
OpenRA.Game/Graphics/SpriteRenderable.cs Outdated Show resolved Hide resolved
OpenRA.Game/Graphics/SpriteRenderable.cs Outdated Show resolved Hide resolved
OpenRA.Game/Graphics/SpriteRenderer.cs Outdated Show resolved Hide resolved
OpenRA.Game/Graphics/Util.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Graphics/DefaultSpriteSequence.cs Outdated Show resolved Hide resolved
@AspectInteractive
Copy link
Contributor

Thanks @pchote for the feedback, will look to make the above changes. I am fairly busy these days so not sure when I will have the time (have some personal matters to deal with as well), but will do it in due course.

@AspectInteractive
Copy link
Contributor

I have addressed all the feedback received so far, as well as rebased to the latest version of ORA as of yesterday.

@reaperrr
Copy link
Contributor

Huh? Why was this closed?

@AspectInteractive2
Copy link
Contributor Author

Huh? Why was this closed?

It closed when I hard reset the branch, I had some issues with rebasing. Have re-opened it now and it is up to date.

@reaperrr
Copy link
Contributor

As far as I'm concerned, this looks good to me once the commits have been squashed (note: something still seems to be a bit wrong with how it was rebased, there seem to be some outdated duplicate commits in the commit history).

One more thing, I noticed that for quantized facings such as turrets, the interpolated facings don't rotate at the exact time as the normal facings, as you can see below. I suspect this might be because QuantizeFacingsFromSprites() runs either before or after InterpolatedFacings are calculated, making them 1 frame off. I'm not sure how to fix this however, since I suspect delaying or speeding up the calculation of InterpolatedFacings is not straightforward (and could have side effects on the Mig and other aircraft that don't use QuantizeFacigsFromSprites()).

My idea would be to add a Rotation field for sequence frames listed under Combine, to give the modder more manual control over which frame to use as base and by how much (and in which direction) to rotate them for additional facings, and then list those along with the default facings. This could also work around the delay issue, as the additional frames would be baked into the sequence definition.
I mean like this:

ca:
	idle:
		Combine:
			ca:
				Frames: 0
			ca:
				Frames: 0
				Rotation: -32
			ca:
				Frames: 1
			ca:
				Frames: 1
				Rotation: -32
			(...)
			ca:
				Frames: 15
			ca:
				Frames: 0
				Rotation: 32
		Facings: 32

But I think that should be left for a follow-up PR, let's get this one merged first.

RoosterDragon
RoosterDragon previously approved these changes Mar 12, 2022
@AspectInteractive2
Copy link
Contributor Author

AspectInteractive2 commented Mar 12, 2022

One minor change not mentioned above in the latest commit. I have now updated UISpriteRenderable.cs to be consistent with SpriteRenderable.cs, as previously it was using float for the rotation parameter, where as SpriteRenderable.cs is now using WAngle. They are now both using WAngle.

I've now reverted this change due to some feedback that UISpriteRenderable.cs shouldn't be using world logic classes such as WAngle. However in order to enable Util.BoundingRectangle() to be used by both classes, this function now uses a float instead of a WAngle, which is actually the right approach since the Util class does not seem to be using any world logic classes at present. All this required is a cast from SpriteRenderable.cs's WAngle to float using the RendererRadians() function, and since this was already being done within Util.BoundingRectangle(), there is no added performance loss here.

OpenRA.Game/Graphics/UISpriteRenderable.cs Outdated Show resolved Hide resolved
OpenRA.Game/Graphics/UISpriteRenderable.cs Outdated Show resolved Hide resolved
OpenRA.Game/Graphics/Util.cs Show resolved Hide resolved
OpenRA.Game/Graphics/Util.cs Show resolved Hide resolved
OpenRA.Game/Graphics/Util.cs Outdated Show resolved Hide resolved
@AspectInteractive2 AspectInteractive2 force-pushed the rotatesprites branch 3 times, most recently from b526166 to 5556de6 Compare April 28, 2022 12:28
@pchote
Copy link
Member

pchote commented Apr 30, 2022

The large interpolated facings don't play nicely with carryalls in d2k:

Before (no interpolated facings):

After:

It looks like the carryall is being slightly rotated while the cargo is remaining fixed in the cardinal direction.

Testcase: The brown carryall in the shellmap.

@AspectInteractive2
Copy link
Contributor Author

AspectInteractive2 commented Apr 30, 2022

The large interpolated facings don't play nicely with carryalls in d2k:

Before (no interpolated facings):
After:
It looks like the carryall is being slightly rotated while the cargo is remaining fixed in the cardinal direction.

Testcase: The brown carryall in the shellmap.

I suspect this is caused by the cargo not having the same InterpolatedFacings trait that the carry-all does, and it not being able to be inherited. Will investigate and report back.

@AspectInteractive2 AspectInteractive2 force-pushed the rotatesprites branch 2 times, most recently from acdc5a2 to e717c7c Compare May 1, 2022 13:41
@AspectInteractive2
Copy link
Contributor Author

Ok so from what I can tell it seems that this is due to the Harvester still having only 32 facings, while the carry-all now has 1024. The fact that the carry-all is slightly tilted only in the interpolated facings version, shows that the tilt in the "After" image above is one of the interpolated facings, which the harvester does not have. Therefore adding more facings to the harvester would solve the issue, but I see that it has the "UseClassicFacings" trait, so it would not be possible unless it was changed to not use classic facings (classic facings are required to only have 32 facings).

@anvilvapre
Copy link
Contributor

#19582.

@Mailaender
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.

None yet