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

Add boolean active to Skybox class in commons. #145

Merged
merged 2 commits into from
Apr 5, 2023
Merged

Add boolean active to Skybox class in commons. #145

merged 2 commits into from
Apr 5, 2023

Conversation

antzGames
Copy link
Collaborator

@antzGames antzGames commented Mar 29, 2023

This PR is for someone using the Mundus runtime, who does not want to render a skybox, or wants to disable/enable skybox rendering whenever they want in their game.

edit: to disable/enable skybox rendering in the scene when ever they want.

@Dgzt
Copy link
Collaborator

Dgzt commented Mar 29, 2023

If you move this variable into Skybox class like this:

public boolean visible = true;

And move the render logic also into Skybox class what uses this logic then you can disable/enable skybox on a screen not just all screen. And later can create settings screen for it in Mundus editor.

@antzGames
Copy link
Collaborator Author

I am open to moving the boolean to to the Skybox class, but not convinced that we should move the skybox render logic to it also.

Given that the rendering of the skybox happens in the Scene class, and you can only render one scene at a time (and therefore only 1 skybox at a time), it doesn't make sense to change the original intent of having the render logic for the entire scene in the Scene class.

It is just an opinion, I will let JTK chime in on what he thinks is best. For me, I just need this functionality for a project I am working on.

@JamesTKhan
Copy link
Owner

JamesTKhan commented Apr 5, 2023

Here is my thoughts. Keep render logic in the Scene for now. If we did move rendering logic out of Scene this is not the PR to do it with. Skybox doesn't derive from GameObject but the way you disable GameObjects is via an boolean active variable.

For consistency lets do the same thing for Skybox. Add public boolean active; to Skybox class then check if skybox.active in Scene.java when rendering it. This will make it easy to tap into via the UI settings later.

The Skybox is public in Scene so the runtime should be able to access scene.skybox.active variable. Will merge PR if you can make that change.

@antzGames
Copy link
Collaborator Author

public boolean active;

added to Skybox.java and

this.active = true;

set in constructor.

In the future if you guys add a active/inactive checkbox in the editor, maybe we should change the Skybox's constructor() and set() methods in the Skybox class to allow you to pass this active boolean parameter to the methods.

Let me know if you would like me to make any other changes.

@antzGames antzGames changed the title Add boolean renderSkybox in commons.Scene Add boolean active to Skybox class in commons. Apr 5, 2023
Copy link
Owner

@JamesTKhan JamesTKhan left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@JamesTKhan JamesTKhan merged commit aa32ced into JamesTKhan:master Apr 5, 2023
@antzGames antzGames deleted the add-renderSkybox-boolean branch April 5, 2023 04:47
@antzGames
Copy link
Collaborator Author

Thanks for approving this PR. Disabling the skybox sets us free. Link below:

https://imgur.com/eBJNIyO

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.

3 participants