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

Ceil function with % and variable works inside editor and with debug build but not with release build #24035

Closed
qarmin opened this issue Nov 28, 2018 · 11 comments · Fixed by #34918

Comments

@qarmin
Copy link
Contributor

qarmin commented Nov 28, 2018

Godot version:
3.1 b243c26

OS/device including version:
Windows 10

Issue description:
My project working inside editor and with debug build but not with release build.
Here is screenshoot with working project, exported with enabled debugging
113

and here is screenshoot with project, exported without enabled debugging
112

Steps to reproduce:

  1. Just export minimal project with debug and without and open it

Minimal reproduction project:
UP.zip

@akien-mga akien-mga added this to the 3.1 milestone Nov 28, 2018
@marcelofg55
Copy link
Contributor

Your project will work if you attach your script to your UP node and replace this lines:

onready var number_of_pages_node : Label = get_node("Tlo/Dokument/OS/IloscStron")
onready var progress_node : TextureProgress = get_node("Tlo/Dokument/OS/CzasDoNastepnejListy")
onready var grid_node : GridContainer = get_node("Tlo/Dokument/SiatkaOfert")

@qarmin
Copy link
Contributor Author

qarmin commented Jan 8, 2019

Seems that something is wrong with ceil function
This is minimal project(QQ.zip) or just as script:

  1. Create Label with this script:
extends Label

func _process(delta : float) -> void:
	var number_of_pages : int = ceil(50)
	set_text(str(5 % number_of_pages))
	
	var newfile : File = File.new()
	newfile.open("/home/rafal/f",File.WRITE_READ) # change directory to correct before start script
	newfile.store_string(str(5 % number_of_pages)) 
  1. Run script and you will see that "5" is print and in file is also written "5"
  2. Export project as Release Build
  3. Run Release project and you will see that "null" is printed and in file is written "7"

As workaround, this line can be change

set_text(str(5 % number_of_pages))

to this:

set_text(str(5 % int(ceil(50)))

@qarmin qarmin changed the title My project working inside editor and with debug build but not with release build Ceil function with % and variable works inside editor and with debug build but not with release build Jan 8, 2019
@qarmin
Copy link
Contributor Author

qarmin commented Jan 10, 2019

Also changing line

var number_of_pages : int = ceil(50)

to this

var number_of_pages : int = int(ceil(50))

fix issue

@vnen
Copy link
Member

vnen commented Jan 15, 2019

Okay, this is kind of a big problem and I'm not sure how to solve it. The original reproduction project is a bit too large but from the comments I noticed where the source of the problem is.

Essentially GDScript tries to coerce the float returned from the ceil() function to an int which is the variable type. The problem is that on a release build a lot of information is stripped, so not only GDScript has no idea about the types involved in a ceil() function, it's also forced to skip a few type checks, resulting in untyped instructions (so it won't try any coercion at all).

Because of that, you have actually a float value but you can't use that with the modulo operator (%), resulting in an error that is silenced in release builds.

The easiest solution is to enforce explicit casting every time. So var my_int : int = 3.14 would always fail and you'd be forced to write var my_int : int = int(3.14), which can get a bit cumbersome.

The other solution would be to always provide type information, even in release builds. That's arguably a good thing, as it can solve a bunch of issues with typed GDScript. I don't know how much this would affect the release build in terms of size though.

We could also store compiled scripts in the export process, instead of just the tokenized source. This would improve the loading process on release, but also would affect scripts loaded on the fly (and lose the ability to just provide the source with a release template). Though this might be a good thing as well, I don't think it would solve the problem.

I'm listening to ideas of what would be the best course of action.

@Zireael07
Copy link
Contributor

QQ.zip from the second-to-last comment is too large? O_o

However, good job figuring out the problem!

@aaronfranke
Copy link
Member

Is there any reason why % can't be made to work with floats? It seems backwards in a dynamically-typed language to make people worry about types. For example, + works with ints and floats.

@vnen
Copy link
Member

vnen commented Jan 29, 2019

It seems backwards in a dynamically-typed language to make people worry about types.

Not really, a dynamically-typed language can still be strongly-typed (as GDScript is). If you try to use the modulo operator on a Transform, for instance, there's not any defined behavior for that, and there shouldn't be. Dynamic type is more about polymorphism than about implicit coercion.

@mnn
Copy link

mnn commented Feb 22, 2019

Not really, a dynamically-typed language can still be strongly-typed (as GDScript is).

Shouldn't a strongly typed language throw an exception/crash when such error occurs?

I might be wrong, but how is type-checking working in build (exported project) when type information is lost? What about dynamically loaded code? Or is type-checking supposed to be only available for a compilation-time, so allowing unsafe (from typing perspective) code execution at runtime? Wouldn't JIT benefit from keeping type info around?

Also modulo can work on floats, it doesn't have to (and IMO shouldn't) coerce values to integer.

@akien-mga
Copy link
Member

Reopening as I reverted #26562.

@jahd2602
Copy link
Contributor

jahd2602 commented Jun 7, 2019

I am in 3.1-stable under Linux and this still happens.

This is a huge issue that shouldn't be ignored because it causes bugs that are very hard to solve, especially for beginners coming from other engines. The fact that it only happens on Release, makes it even harder to debug.

In our team, we spent the best part of a week trying to find a bug related to this issue, instead of implementing features for our game.

@bojidar-bg
Copy link
Contributor

Discussing this on IRC, seems like the best solutions would be:

  • Adding type information to release builds, as long as it does not cause the resulting binary to grow too much.
  • Adding a mode to GDScriptCompiler in which it emits valid code even when type information is not available. That would mean assuming all engine functions are untyped.
    • If this causes too much of a performance impact, compiling GDScript to opcodes instead of just saving the source tokens should help.

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