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

Improve memory management of treads #4640

Merged
merged 7 commits into from
Jan 29, 2023

Conversation

Garanas
Copy link
Member

@Garanas Garanas commented Jan 26, 2023

An interesting one, every time a unit would:

  • Transition to or from 'Stopping' movement state
  • Transition to a new terrain type
  • Get beamed up / down from a transport

A unit with treads would:

  • Re-allocate the TreadThreads table multiple times
  • Kill all threads, if they exist
  • Allocate new threads, if we need them

And then something like this happens, that constantly triggers the transition events:

bumping.mp4

On top of that, this pull request decreases the time that a splat (a tread mark) exists by tech while increasing the LOD by tech. See also:

@Garanas Garanas added this to the Development iteration I milestone Jan 26, 2023
@Garanas Garanas added area: sim Area that is affected by the Simulation of the Game area: graphics Anything Related to the Game Graphics labels Jan 26, 2023
@Garanas
Copy link
Member Author

Garanas commented Jan 26, 2023

Note that the system doesn't actually work properly: try moving 200 tanks at once and you'll see what I mean. There appears to he a maximum to the number of splats being rendered, and that maximum is relatively low in comparison to the number of splats generated.

@BlackYps
Copy link
Contributor

I think that's the same reason why building tarmacs etc. disappear sometimes when tilting the camera. The render limit kicks in. I guess they implemented it to not stress the hardware too much, but today it shouldn't be an issue anymore. Probably needs an engine patch to increase it though.

Do I see this correctly that different tread textures per terrain type are possible?

@Hdt80bro
Copy link
Contributor

Yes, any effect for any terrain type can be changed, see https://github.com/FAForever/fa/blob/deploy/fafdevelop/lua/TerrainTypes.lua

@Garanas
Copy link
Member Author

Garanas commented Jan 27, 2023

I think that's the same reason why building tarmacs etc. disappear sometimes when tilting the camera. The render limit kicks in. I guess they implemented it to not stress the hardware too much, but today it shouldn't be an issue anymore. Probably needs an engine patch to increase it though.

Tarmacs are decals, they work different. I'm not entirely sure what the exact difference is.

@Garanas
Copy link
Member Author

Garanas commented Jan 27, 2023

Do I see this correctly that different tread textures per terrain type are possible?

Yes, but it didn't quite work like that. At the moment terrain would only disable treads (no need for treads on rocks or metal, for example). As a result the majority of generated maps have no treads. I'm wondering whether that is why generated maps always felt faster than regular maps. With this pull request we change that - treads always work, even when the terrain type tells you not to.

@BlackYps
Copy link
Contributor

generated maps have no tread marks because no terrain type is set. The fix for that should be to set an appropriate terrain type on generation, because there are more effects that also depend on the terrain type, not enforcing tread marks on all terrain.
Unless you want to have it always enabled for a different reason.

Copy link
Contributor

@Hdt80bro Hdt80bro left a comment

Choose a reason for hiding this comment

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

It doesn't look like this will work. Adding back the missing function is trivial, but letting treads underwater, on snow, and on tarmacs looks very unnatural. Since threads are very lightweight, I'm interested in the performance gains. You might be able to get more by consolidating all treads for the unit into one thread, instead of one for each. The Fatboy treads also looks very noisy since it always overlaps both full treads, so I'm not sure that's better.

lua/sim/Unit.lua Show resolved Hide resolved
SuspendCurrentThread()
self.TreadSuspend = nil
WaitTicks(interval)
Copy link
Contributor

Choose a reason for hiding this comment

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

This delays tread creation by one interval any time the unit moves again

Copy link
Member Author

Choose a reason for hiding this comment

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

Visually that is fine - during the first interval the unit barely moves.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can notice it--it's logically an error, just remove it

Copy link
Member Author

Choose a reason for hiding this comment

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

adjusted it to waiting just a single tick

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you think you need it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Having an infinite loop with no wait statement is asking for trouble.

@Garanas
Copy link
Member Author

Garanas commented Jan 27, 2023

@Hdt80bro I partially understand.

Adding back the missing function is trivial, but letting treads underwater, on snow, and on tarmacs looks very unnatural

I agree with the water, but treads have always shown on tarmacs. See also:

image

Testing on Arctic Refuge and you're correct: the treads do not show on snow. Odd - in my personal perception they should be visible on snow too, but maybe a different type of tread. I'll add in the terrain type check again.

image

You might be able to get more by consolidating all treads for the unit into one thread, instead of one for each.

I like the idea, but that would be something for a second iteration. This first iteration is already a good improvement memory wise on the current implementation.

@Hdt80bro
Copy link
Contributor

Ah, my bad, these are left over from vanilla
image
Still, not making tread marks on regular concrete, underwater, on snow makes sense (treads look like the dirt being kicked up, something ice and snow doesn't have--some snow types looks like they're only a thin layer and we could change them, but the tread would probably still look different than normal)

@Garanas
Copy link
Member Author

Garanas commented Jan 27, 2023

We could look into adjusting the terrain type:

image

But how do we change them back, after the tarmac is gone 🤔

@Garanas
Copy link
Member Author

Garanas commented Jan 27, 2023

To me the fatboy doesn't look that bad

image

@BlackYps
Copy link
Contributor

But how do we change them back, after the tarmac is gone

Maybe on destruction of the building, when the tarmac is set for removal with a waiting time period. It's not perfect, as treads would show up as soon as the building is destroyed, but while the building is still alive it would work.

@Garanas Garanas force-pushed the deploy/fafdevelop branch 2 times, most recently from d0a77bd to 4c8aca6 Compare January 28, 2023 20:17
@Garanas
Copy link
Member Author

Garanas commented Jan 29, 2023

@BlackYps it wasn't so much about when to change it, more about what to change it (back) to?

@Garanas
Copy link
Member Author

Garanas commented Jan 29, 2023

I'll be merging these changes in as they are a good improvement over the old implementation, without changing the old behavior.

@Garanas Garanas merged commit f2d8bd2 into deploy/fafdevelop Jan 29, 2023
@Garanas Garanas deleted the refactor/tread-management branch January 29, 2023 12:44
@Garanas Garanas mentioned this pull request Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: graphics Anything Related to the Game Graphics area: sim Area that is affected by the Simulation of the Game
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants