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

Lighting fixes #1929

Merged
merged 6 commits into from
Aug 29, 2020
Merged

Lighting fixes #1929

merged 6 commits into from
Aug 29, 2020

Conversation

HarsimranVirk
Copy link
Contributor

We have 4 fixes here,

  1. Removal of unnecessary undefined handling of gdjs.LightObstaclesManager.
  2. Moving the handling of light texture to pixi renderer.
  3. A really weird bugfix of debugging mode's graphics. I don't know what happened, but this is how the pixi graphics was being rendered:
    Screenshot (1)

Neither I understand why this happened, nor I'm sure about why this particular fix works 😓. All I did was randomly change the order of lineTo and moveTo calls, and it worked. I'm guessing this is really specific to pixi, as we've upgraded the version as well, but arguably it was a minor change.

  1. Also changed the algorithm a bit (specifics about it in the review below) to have that expanding boundary box of light.

Copy link
Owner

@4ian 4ian left a comment

Choose a reason for hiding this comment

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

Sorry got sidetracked by other reviews! I've added a few comments but it looks good otherwise!
We'll merge this once you've addressed them.

In terms of performance, take a look at minimizing the number of calls to Math.sqrt/Math.hypot/Math.pow (or manually a*a) as it's fairly expensive when called repeatedly (for example, work with squared distance as much as possible, and take the square root only at the last step).

Extensions/Lighting/lightruntimeobject.js Show resolved Hide resolved
Extensions/Lighting/lightruntimeobject-pixi-renderer.js Outdated Show resolved Hide resolved
@4ian 4ian added the 🏁PR almost ready: final fixes The PR is almost ready and needs final fixes and/or polishing before merge label Aug 24, 2020
@HarsimranVirk
Copy link
Contributor Author

@4ian this dynamic polygon fix has made the light objects really good 😬. Earlier, having "floors" as light obstacles was a disaster, as the "floor" can be really long. Now, since the bounding box is dynamic, this case is handled as well :)

Screenshot (2)

Note that even if the bounding box is really large, the number of rays remain the same. So, the only extra performance cost here is the resizing of the polygon :)

@4ian
Copy link
Owner

4ian commented Aug 26, 2020

this dynamic polygon fix has made the light objects really good 😬. Earlier, having "floors" as light obstacles was a disaster, as the "floor" can be really long. Now, since the bounding box is dynamic, this case is handled as well :)

Indeed, I've never thought about this case - while it's very common.
Note that there is still a case where things can be glitching: when an obstacle is partially inside another. In this case, the algorithm is not properly seeing the vertice formed by the intersection of the hitboxes. Unless you can find of a solution, we'll leave this as it is for now I guess.
(With this bugfix, you've avoided this issue by enlarging the bounding box to always include the obstacles, so no unseen intersections anymore.)

Note that even if the bounding box is really large, the number of rays remain the same. So, the only extra performance cost here is the resizing of the polygon :)

We indeed keep the same number of rays which is great (performance will be limited by the number of objects and the complexity of their hitboxes, as before). So seems this "bugfix" has no adverse consequence on performance :)

@Bouh
Copy link
Collaborator

Bouh commented Aug 28, 2020

Can you add the button for "Add lighting layer" next to the button for "Add a layer" on left please ? We waste the space for no reason, this worried me.

image

@4ian
Copy link
Owner

4ian commented Aug 29, 2020

I asked for the opposite a while ago 😅 Was because of small screens. Otherwise we can use a responsive component.

@HarsimranVirk
Copy link
Contributor Author

HarsimranVirk commented Aug 29, 2020

I guess we can use a split button in this case?

EDIT: Let's do that in another PR (not to add unrelated ui stuff in this PR), we might as well define a new ui element to be used in IDE 😬.

@4ian
Copy link
Owner

4ian commented Aug 29, 2020

Let's do that in another PR (not to add unrelated ui stuff in this PR), we might as well define a new ui element to be used in IDE 😬.

Yes let's do that in another PR! :)

@4ian 4ian merged commit 5d091c0 into 4ian:master Aug 29, 2020
@4ian
Copy link
Owner

4ian commented Aug 29, 2020

Merged! :)

@HarsimranVirk HarsimranVirk deleted the lighting-fixes branch January 19, 2021 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏁PR almost ready: final fixes The PR is almost ready and needs final fixes and/or polishing before merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants