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

feat(demo): make teleport "beam" visible only if teleport is active #445

Merged
merged 3 commits into from
Apr 14, 2023

Conversation

toedter
Copy link
Contributor

@toedter toedter commented Apr 10, 2023

When I was playing around with the teleports in the demo,
I noticed that there is no visible clue if they are active or inactive.

This PR makes the cylinder containing the "beam" only visible if the teleporter is active.

What do you think?

@BastiaanOlij
Copy link
Member

I think that is a nice change, though I would probably change the color. @Malcolmnixon whats your input on this?

@BastiaanOlij BastiaanOlij added the enhancement New feature or request label Apr 12, 2023
@Malcolmnixon
Copy link
Collaborator

I just want to confirm the intent a little better.

Back in January pietru2004 did a pull request (#321) where he wanted to control what teleporters were present by using the show/hide visibility toggle. Hiding a teleporter would have the effect of:

  • Disabling all colliders and teleportation effects
  • Removing from the teleporter ring-sorting in the main level

This seems to be switching over to using the teleporter "active" bool to disable the teleporter while still keeping it visible and in the ring-sorting.

I'm actually OK with either (or even both) approaches. I am curious as to why we're enhancing the teleporters with enhanced visibility and enable/disable functionality? I don't think we actually disable any of them in the demos - although I guess we could make a demo where you have to somehow toggle on or off a teleporter.

@toedter
Copy link
Contributor Author

toedter commented Apr 12, 2023

The main reason for this MR is that Godot beginners (like myself) might take a look at the demos and start using existing elements for their own games. Since the use case that a teleport is activated on a game event (like collecting all coins or solving a puzzle) is very common, I thought it would be a good idea if the teleport would support this out of the box, even though it is currently not used in the demos.

@BastiaanOlij regarding the color change. I also like this idea, but I think it is probably more intuitive if no beam is shown in inactive state. For example, consider you have an active and an inactive teleport, one is blue, the other green. As a player (who did not read the manual :)) only try and error will get the answer which one is active.

But I think a variable to set the beam color would be great (independent of the state). May be even more options, like:

  • beamActiveColor: color
  • beamInactiveColor: color
  • beamVisibleWhenInactive: bool

@BastiaanOlij
Copy link
Member

The main reason for this MR is that Godot beginners (like myself) might take a look at the demos and start using existing elements for their own games.

Indeed that was my first thought as well. If you use the teleporter in a game, say you use our godot-xr-template as a starting point, I can see that you would keep a teleporter disabled but visible until a puzzle is solved or some action is performed.

As for your beam settings, sounds good. We don't use it enough because it's only been added in Godot 4 when using GDScript, but you can now create property groups to make the properties less busy.

@toedter
Copy link
Contributor Author

toedter commented Apr 12, 2023

I have added the following:

  • the teleport beam shader now has a parameter beam_color
  • the teleport has 3 more variables
    • Active Beam Color: the beam color in active state (defaults to the original beam color)
    • Inactive Beam Color: the beam color in inactive state (defaults to a dark red)
    • Inactive Beam Visible: When enabled, the beam is always visible and has either the active or the inactive color. Default is false

Now all the above use cases can be easily configured.

@Malcolmnixon
Copy link
Collaborator

Currently all teleporter instances share the same shader material instance, so changing one of them affects all of them. For example if you pick one of the teleporters in main_menu_level.tscn and deactivate it, they all change red.

You can fix this by going into teleport.tscn, then picking the Teleport Area cylinder, selecting the Surface Material Override material, and checking its "Local to Scene" checkbox. You'll then find that modifying any of the teleporters in the main_menu_level.tscn doesn't affect the others.

@toedter
Copy link
Contributor Author

toedter commented Apr 13, 2023

Thx @Malcolmnixon, it is fixed now.

Copy link
Collaborator

@Malcolmnixon Malcolmnixon left a comment

Choose a reason for hiding this comment

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

Loogs good to me.

@BastiaanOlij BastiaanOlij merged commit 94e7b31 into GodotVR:master Apr 14, 2023
@BastiaanOlij BastiaanOlij added this to the 3.5.0 milestone Apr 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants