Skip to content

Change lua_pushnumber to lua_pushinteger for non floating point values - #4523

Draft
xmish wants to merge 1 commit into
otland:masterfrom
xmish:lua_integer
Draft

Change lua_pushnumber to lua_pushinteger for non floating point values#4523
xmish wants to merge 1 commit into
otland:masterfrom
xmish:lua_integer

Conversation

@xmish

@xmish xmish commented Sep 1, 2023

Copy link
Copy Markdown
Contributor

Pull Request Prelude

Changes Proposed

Replace some lua_pushnumber with lua_pushinteger to fix display of some integer values in game.
I'm not sure if it's a valid approach and also I'm not sure about changes in:

  • setField
  • LuaScriptInterface::registerVariable

Issues addressed:
#4522

@ghost
ghost requested review from a team September 2, 2023 02:28
@ranisalt
ranisalt marked this pull request as ready for review September 2, 2023 23:03
@ranisalt
ranisalt marked this pull request as draft September 2, 2023 23:03
@ranisalt

ranisalt commented Sep 2, 2023

Copy link
Copy Markdown
Member

Sorry missclicked

@EvilHero90

Copy link
Copy Markdown
Contributor

needs a rebase

@EvilHero90 EvilHero90 added enhancement Increase or improvement in quality, value, or extent decisions Debatable/disputable labels Jun 2, 2024
@EvilHero90 EvilHero90 added this to the 1.8 milestone Jun 2, 2024
@ranisalt

ranisalt commented Jun 2, 2024

Copy link
Copy Markdown
Member

@EvilHero90 we can't merge this right now because it implies dropping LuaJIT support, but we can do it for the next release cycle

@EvilHero90

Copy link
Copy Markdown
Contributor

@EvilHero90 we can't merge this right now because it implies dropping LuaJIT support, but we can do it for the next release cycle

There was no intention to push this immediatly, I just want to see if this gets worked on or not

@Codinablack

Copy link
Copy Markdown
Contributor

If this does get worked on, it is worth noting that "setField" uses "lua_pushNumber" and is probably actually needing to push a decimal in some places where that method is used, but also in many places it probably shouldn't. I believe that "setField" should be changed to "setIntField", "setFloatField" (yes I know it's actually a double), and "setStringField" or something along those lines, just to be on the safe side.

@Codinablack

Copy link
Copy Markdown
Contributor

I ended up using a metaprogramming approach with concepts and templates to create the additional setField overloads that are needed when making this sort of change, I recommend it highly over my previous recommendation to make new methods called setIntField, ect.

@alysonjacomin

Copy link
Copy Markdown
Contributor

Hi guys, this seems to have been forgotten at some point, we could do a new analysis on these changes to merge into the main-code

@ArturKnopik

Copy link
Copy Markdown
Contributor

luaPlayerGetDeathPenalty is pushing player->getLostPercent() that is double but this function is not used in TFS scripts (getPlayerLossPercent and Player:getDeathPenalty() )
rebase is required
changes for the new code are also needed, currently we have 330 calls of lua_pushnumber, this PR changes 316 of them

@alysonjacomin

Copy link
Copy Markdown
Contributor

@ArturKnopik Yeah, this PR was made in 2023, we have to do a new analysis to see what should be done, it is important to highlight that in the Issue topic related to this PR, Nekiro reported some risks regarding these changes

@ranisalt

Copy link
Copy Markdown
Member

I'm gonna refresh this

@ranisalt ranisalt self-assigned this Jun 16, 2025
@xmish

xmish commented Jun 16, 2025

Copy link
Copy Markdown
Contributor Author

Im about to re-check it this weekend

@MillhioreBT

MillhioreBT commented Jun 16, 2025

Copy link
Copy Markdown
Contributor

I made this change a long time ago in the TFS downgrade engine using lua5.4, and I can confirm everything is covered. We just need to fix the conflict, and it'll be ready for review/merge.

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

Labels

decisions Debatable/disputable enhancement Increase or improvement in quality, value, or extent

Projects

Status: In review

Development

Successfully merging this pull request may close these issues.

7 participants