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 #1698, reverting units to nanoframe via Lua #1864

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

sprunk
Copy link
Collaborator

@sprunk sprunk commented Dec 30, 2024

No description provided.

@sprunk sprunk added the status: candidate PRs that should be good to go or important for next release label Jan 3, 2025
Copy link
Collaborator

@saurtron saurtron left a comment

Choose a reason for hiding this comment

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

Tested and seems to work fine (although maybe not completely, see below comments), although not sure if it can be missing something, as some of the operations at FinishedBuilding don't seem to be reversed, for example ChangeLos or envResHandler callins, also doesn't have an eoh, while other callins usually have it when changing unit status.

Just for the record, don't think it's a blocker since any shortcomings will likely come up with usage.

@saurtron
Copy link
Collaborator

saurtron commented Jan 6, 2025

Something else I noticed, if I don't call SlowUpdate() at the beginning of the function (just after the beingBuilt check) then weird things happen like not all units render properly but may have map icon (could be because I create and revert to nanoframe very fast (4 frames), if I wait more (20) frames before revert then it works ok).

left is with slowupdate call, right without (otherwise it's the same scenario):

frames

@sprunk
Copy link
Collaborator Author

sprunk commented Jan 6, 2025

ChangeLos

Seems so. On some level this is good because games would be able to keep nanoframes able to see, but it looks like nanoframes don't have LoS anyway:

if (unit->isDead || unit->beingBuilt)
return;

envResHandler

Good find. This (and all other feedback here) applies to the existing method of reverting units to nanoframes via reclaim so should be fixed at some point even if this PR doesn't go through. #1883

eoh

Eoh looks inconsistently applied, some callins have it and some don't. It didn't have it previously so I kept it as-is, it would probably be fine to have since the event is relatively rare but impactful.

not all units render properly

BAR has custom rendering which probably assumes units can't revert to nanoframes so this is somewhat expected. Native engine nanoframe drawing works.

@saurtron
Copy link
Collaborator

saurtron commented Jan 8, 2025

BAR has custom rendering which probably assumes units can't revert to nanoframes so this is somewhat expected. Native engine nanoframe drawing works.

I'm not convinced, I did add support for ReverseUnitBuilt so wireframe rendering of reverted units would work properly.

Also I did test without cus_gl4 (I think it's the one doing the custom rendering, as wireframe shows normal without that). Also tried forcing dependency test to return false for gl4, that forces even more custom rendering wupgets to be disabled.

Here shows the same problem with default wireframe drawing (that's a 10x10 solar array, but some don't show without the slowupdate call):

wireframe

In case you want to take a look I uploaded my changes to test this here, added UnitReverseBuilt support for cus_gl4, gl4_unit_tracker and widget, also added a test test_wireframe so can be tested with /runtests wireframe with bar. All included at that branch, where cus_gl4 is disabled too.

The problem might be triggered by the amount of units and very fast reverting to wireframe since unit creation, rather than custom rendering. Not totally sure about this but looks worth investigating.

Not saying it's not a bar specific issue, but it might not be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: candidate PRs that should be good to go or important for next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants