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

Update CollideCheck.hx #1095

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

Update CollideCheck.hx #1095

wants to merge 2 commits into from

Conversation

octplane
Copy link

@octplane octplane commented Aug 20, 2022

  • Remove Dead code
  • Move var setup at beginning of functions
    - Replace clever but complex division by two with explicit division
  • Rotate entity later to ensure collision is computed with this frame's rrect rotation

- Remove Dead code
- Move var setup at beginning of functions
- Replace clever but complex division by two with explicit division
- Rotate entity later to ensure collision is computed with this frame's `rrect` rotation
@Yanrishatum
Copy link
Contributor

Replace clever but complex division by two with explicit division

Any reason for it? Bitshift is much better for integer division by power of twos as it's much cheaper to calculate compared to actual division, which is always a much more expensive operation, especially in case of Haxe where it does not have an integer division support and would cause a floating point division. And using /2 in this case would cause rrect position to be .5 shifted if window/scene size is not divisible by 2.
I don't see what's so complex or clever about using bitshift in such situation.

@octplane
Copy link
Author

I wanted to change that for several reasons:

  • Using bitshift for integer division usually implies that you know the underlying encoding of numbers in your language. For this case, we can find (more or less quickly) that s2d.width is an Int but this could be encoded in any way in the language
  • In modern languages, it's pretty rare to use bitshift instead of integer division in general (at least in the non-gaming/demoscene/time-constrained industry)
  • In other simpler samples, the division is very explicit https://heaps.io/samples/base2d.html and not confusing
  • This sample is about collision check and I wanted all the rest of the code to be very straightforward in order not to confuse beginners

Maybe we could use both forms and explains that simple optimization in a comment?

@Yanrishatum
Copy link
Contributor

Heaps only targets HL VM, JS and C, 2 of which would use integer for numbers and JS is "who the hell knows" (or rather "number" can be stored both as an integer or a float, and using bitwise operators ensures it is an integer).

In general it's a fair reasoning, and I was mainly curious on why change a piece of code that works just fine. I still think that integer division by 2s should use bitshift, because division is goddamn expensive compared to much, MUCH simpler binary shift operation, and people (especially ones using modern languages, especially JS) need to learn to write performant code. ;)

@octplane
Copy link
Author

people (especially ones using modern languages, especially JS) need to learn to write performant code.

I wouldn't know, I let my compiler optimize for me and I never write JS :-D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants