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

Compiler Crash(?) when trying to add More If/Else Braches #3987

Closed
michaelcrichlow opened this issue Jul 27, 2024 · 8 comments
Closed

Compiler Crash(?) when trying to add More If/Else Braches #3987

michaelcrichlow opened this issue Jul 27, 2024 · 8 comments

Comments

@michaelcrichlow
Copy link

Context

Please provide any relevant information about your setup. This is important in case the issue is not reproducible except for under certain conditions.

  • Operating System & Odin Version:
    Odin: dev-2024-07:e7d37607e
    OS: Windows 10 Home Basic (version: 22H2), build 19045.4651
    CPU: Intel(R) Core(TM) i7-10700K CPU @ 3.80GHz
    RAM: 32646 MiB

INFO: > Vendor: NVIDIA Corporation
INFO: > Renderer: NVIDIA GeForce GTX 1070 Ti/PCIe/SSE2
INFO: > Version: 3.3.0 NVIDIA 536.67
INFO: > GLSL: 3.30 NVIDIA via Cg compiler

Expected Behavior

I just wanted to add more if/else statements to complete the desired logic (simple but effective).

Current Behavior

When I add more than a certain amount of if/else statements, Odin doesn't run. You can see a brief video of my experience here:
https://www.youtube.com/watch?v=8BoTwDh6b7w

Failure Information (for bugs)

Please help provide information about the failure if this is a bug. If it is not a bug, please remove the rest of this template.

Steps to Reproduce

I added the project I was working on here: https://github.com/michaelcrichlow/odin_07_14_2024

The file that was causing the issue is here: https://github.com/michaelcrichlow/odin_07_14_2024/blob/main/additional_functions.odin

Uncommenting any more if/else statements in the 'get_tile_selector_position' proc causes Odin not to run.

Failure Logs

A red X with a circle around it flashes to the left of the terminal line when Odin is run and then goes away really quickly. (It doesn't stay so I can investigate, hover over it, etc.)

@Kelimion
Copy link
Member

Kelimion commented Jul 27, 2024

128 lines -> 9 lines for the mouse procedure. Same output.

get_mouse_block_and_position_b :: proc(is_fullscreen: bool, x: f32, y: f32) -> (block: int, position: rl.Vector2) {
	scale := 4 if is_fullscreen else 1

	if y >= f32(0 * scale) && y < f32(16 * scale) && x < f32(320 * scale) {
		block  = 1 + int(x / f32(scale * 16))
		position.x = f32(block - 1) * 16
	}
	return
}

There are similar constant offsets and factors to take advantage of in get_tile_selector_position that would allow you to rewrite those 2400 lines to about 10 as well.

@michaelcrichlow
Copy link
Author

Thank you! I figured there was a better way to do it but I couldn't figure it out. Thanks very much and I'll try to refactor it using your method! Thanks again! :)

@Kelimion
Copy link
Member

Kelimion commented Jul 27, 2024

Made one small update and put the block calculation in the condition as well so it stays 0 if x >= 320 * scale.

Good luck on retooling the big procedure. At a quick glance it'll be a similar story of dividing x by either 16 or 64, using that as an integer (rounded) factor, and then together with an offset of -2 using that for the new x coordinate. And a similar story for y.

Try putting both x and y in a nested loop that you feed the procedure and print the result if the output from the old and new versions don't agree so you can spot how much you're off by.

@michaelcrichlow
Copy link
Author

Thanks again! It was the kick I needed to redo the logic in much more sane manner. >_<

get_tile_selector_position_b :: proc(is_fullscreen: bool, x: f32, y: f32) -> (i32, i32) {
	pos_x: i32
	pos_y: i32
	scale: i32 = 1
	if is_fullscreen do scale = 4

	for i: i32 = 0; i < 640; i += 16 {
		for j: i32 = 0; j < 360; j += 16 {
			if x >= f32(i * scale) &&
			   x < f32((i + 16) * scale) &&
			   y >= f32(j * scale) &&
			   y < f32((j + 16) * scale) { 	// start here -------------
				return (i - 2), (j - 2)
			}
		}
	}
	return 0, 0
}

This works and takes care of the entire screen like I wanted. Thanks again! :)

@Kelimion
Copy link
Member

Kelimion commented Jul 27, 2024

That's a lot better already. For fun try a version _c that divides the incoming x and y by the 16 or 64 (depending on fullscreen) so you're left with rounded integer value for each that's downscaled. And then you return x_scaled - 2, y_scaled - 2.

It's what I did with the mouse position: scaled the grid and rounded the divided number by turning it into an int, then scaled back and offset as required.

No need for a nested loop for the calculation itself, but you keep the condition you have in your inner loop to check if the coordinates are within bounds, so you can return 0, 0 if they're not.

And then to sanity check you feed both versions the same input for all coordinates:

for i: i32 = 0; i < 640; i += 1 {
	for j: i32 = 0; j < 360; j += 1 {
		x1, y1 := version_b(true, i, j) // fullscreen logic
		x2, y2 := version_c(true, i, j) // fullscreen logic
		if x1 != x2 || y1 != y2 { print out i, j and output from both to suss out where the logic broke }
	}
}
// same again for fullscreen = false

@michaelcrichlow
Copy link
Author

Just following up. Although my code was rewritten with a lot more sense (thanks again!), was it ever discovered exactly what was happening with the original version?

@Kelimion
Copy link
Member

Kelimion commented Aug 8, 2024

It's a recursive descent parser, and so you exhausted the stack limit. That can be addressed, and it's why I left the issue open.

@michaelcrichlow
Copy link
Author

Ah, I see. Thanks for the clarification. Since this I haven't had any issues, so that's cool. Really enjoying Odin! 👍

gingerBill added a commit that referenced this issue Aug 24, 2024
…switch` statement or refactor the code to not use a large if-else chain
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants