search

tessarakkt / godot4-oceanfft

Tessendorf FFT based ocean waves and buoyancy in Godot 4 using compute shaders
MIT License
372 stars 18 forks source link

Improve Plugin End-User Ergonomics #11

Open Tattomoosa opened 9 months ago

Tattomoosa commented 9 months ago

Great work! But there are some design decisions which hamper end-user usability, notably:

Hardcoded node paths in scripts which define rigid scene structures and have no configuration warning when not properly configured

I've made a commit in a fork that I can PR which I think nicely solves this for BuoyancyBody3D and BuoyancyProbe3D, as well as one that removes the hardcoded paths in MotorVessel3D (just opting for exported node paths). (https://github.com/Tattomoosa/godot4-oceanfft)

This has the added benefit that probes added during runtime will work without adding them manually as a separate step. (And it just occurred to me that they could easily remove themselves when they exit the tree, so I'll make a commit for that as well)

No in-editor preview which would be very useful for adjusting parameters

Poking at this but having some problems getting QuadTrees to play nice as a tool script, still looking at that.

MotorVessel3D can only be controlled by the keyboard

This is pretty easy to resolve and seems worth doing to allow for autonomous vessels, but considering going a bit further and supporting multiple propellers while I'm at it. Apparently twin rudders is a thing too, TIL.

Necessary for end user to add, setup, and link several nodes to use

This has some other considerations:

  1. It's probably not ideal to extend WorldEnvironment into OceanEnvironment for a couple reasons
    1. Environments can be added to cameras directly to override WorldEnvironment and this behavior should be considered
    2. Other plugins may extend WorldEnvironment
    3. Multiple oceans can't have different properties
      • Seems like a user may potentially want to add more than one, e.g. a beach resort setting that also has a pool
  2. As-is, properties in several nodes need to be modified to tweak ocean appearance
    • Nodes that need to be linked and potentially need modifications:
      1. QuadTree3D
      2. Ocean3D
      3. OceanEnvironment

It seems to me the ideal end-user experience would be to add a single node that just works, but I'm not really sure that's reasonable yet. Happy to keep poking and implement changes myself but wanted to start a conversation and get your thoughts before I do too much work -- would definitely prefer to contribute here than maintain a fork.

tessarakkt commented 9 months ago

There's no question that things need to be improved, this is very much still a work in progress and has mostly just been hashing out the basic functionality of things.

I haven't had a chance to properly look over the changes you published, or test them out, but overall I generally agree with everything you are saying.

Hardcoded node paths in scripts which define rigid scene structures and have no configuration warning when not properly configured

Agree with this, and from what I've seen of the code, the changes look good as well.

No in-editor preview which would be very useful for adjusting parameters

Agree this is a pain, I recall there being a reason I didn't bother trying to make this work, but I can't remember what that reason was. If Terrain3D works as well as suggested in the other issue, I'd much rather ditch the QuadTree3D and take advantage of that. I was already looking at integrating with that plugin for shoreline interactions and such.

MotorVessel3D can only be controlled by the keyboard

MotorVessel3D overall is a very crude implementation, and was mostly only meant as just enough to be able to scoot around on the ocean I made :P.

In a final complete implementation, there definitely needs to be support for multiple propellers, and they may be in many different positions or orientations (bow thrusters for example), propellers could pivot for steering eliminating the need for rudders (or complement them), and propellers could either be fixed pitch or variable pitch.

It's probably not ideal to extend WorldEnvironment into OceanEnvironment for a couple reasons

I'm not super fond of the implementation of OceanEnvironment overall, open to alternatives.

As-is, properties in several nodes need to be modified to tweak ocean appearance

There are a lot of places that need to be touched to change the ocean. I haven't been worried too much about that so far as things are still being added and rearranged (when I have time), so I haven't spent too much time consolidating where to configure things.

would definitely prefer to contribute here than maintain a fork

I would be happy to have any contributions and pull requests you are willing to make.

Tattomoosa commented 9 months ago

Agree with this, and from what I've seen of the code, the changes look good as well.

Great! While I'm trying to keep each commit clean, definitely view my fork more as me tinkering with ideas, then once we decide on specific plans I'll make clean PRs here. Happy to do that with the buoyancy change now if you'd like?

(Re: In-Editor Preview) Agree this is a pain, I recall there being a reason I didn't bother trying to make this work, but I can't remember what that reason was. If Terrain3D works as well as suggested in the other issue, I'd much rather ditch the QuadTree3D and take advantage of that. I was already looking at integrating with that plugin for shoreline interactions and such.

I think it makes a lot of sense to integrate with Terrain3D if the user has installed it but I think it's preferable to not require it so I think this plugin should ship its own LOD solution as well, maybe preferring Terrain3D's solution if available.

MotorVessel3D overall is a very crude implementation, and was mostly only meant as just enough to be able to scoot around on the ocean I made :P.

In a final complete implementation, there definitely needs to be support for multiple propellers, and they may be in many different positions or orientations (bow thrusters for example), propellers could pivot for steering eliminating the need for rudders (or complement them), and propellers could either be fixed pitch or variable pitch.

Cool. Yeah, no worries - I was just a little unsure of your intent since it's reasonable to consider motor vessels out-of-scope and it seemed more like an example implementation. That said, IMO providing a solution is preferable.

It might make sense to do something like the existing BuoyancyBody3D/BuoyancyProbe3D relationship, eg MotorVesselPropeller3D/MotorVesselRudder3D, where then MotorVessel3D provides steering, with each propeller/rudder having constraints defined on that object. That seems flexible/powerful to me, but solving for the steering properly might get complicated. What do you think?

It's probably not ideal to extend WorldEnvironment into OceanEnvironment for a couple reasons

I'm not super fond of the implementation of OceanEnvironment overall, open to alternatives.

Yeah I didn't suggest a better solution because I don't have one yet... I think a replacement just needs access to the camera and its environment to function and from there it's mostly a question of exposing that in a way that makes sense.

As-is, properties in several nodes need to be modified to tweak ocean appearance

There are a lot of places that need to be touched to change the ocean. I haven't been worried too much about that so far as things are still being added and rearranged (when I have time), so I haven't spent too much time consolidating where to configure things.

Yeah that makes sense. It's possible to still have that development flexibility and expose a node to the end user that does nothing but have all the settings that should be exposed centralized and sets everything else up properly using internal children. That also minimizes what a user might have to do when updating the plugin.

Going work on getting a preview working in the editor now. I'm pretty useless as far as shaders go but if you have any further thoughts on the user interface side of things that I haven't brought up let me know, I'm happy to work on implementing that side of things.

tessarakkt commented 9 months ago

Agree with this, and from what I've seen of the code, the changes look good as well.

Great! While I'm trying to keep each commit clean, definitely view my fork more as me tinkering with ideas, then once we decide on specific plans I'll make clean PRs here. Happy to do that with the buoyancy change now if you'd like?

Sounds good! And yes you can do up the buoyancy change at your convenience.

I think it makes a lot of sense to integrate with Terrain3D if the user has installed it but I think it's preferable to not require it so I think this plugin should ship its own LOD solution as well, maybe preferring Terrain3D's solution if available.

That would be the reasonable path to go.

It might make sense to do something like the existing BuoyancyBody3D/BuoyancyProbe3D relationship, eg MotorVesselPropeller3D/MotorVesselRudder3D, where then MotorVessel3D provides steering, with each propeller/rudder having constraints defined on that object. That seems flexible/powerful to me, but solving for the steering properly might get complicated. What do you think?

If you're familiar with the game Stormworks, that is a good example of what I would think. Building each of the components with their basic function, and the placement and control of those components (including compound steering if its desired) is up to the user to define.

Having the steering be automatically calculated for a simple WASD input may be desired for simple arcade style games, but if someone is trying to build more of a ship simulator type game, then the player would expect to have much finer control over each of the propellers/rudders individually.

It's possible to still have that development flexibility and expose a node to the end user that does nothing but have all the settings that should be exposed centralized and sets everything else up properly using internal children. That also minimizes what a user might have to do when updating the plugin.

That's definitely something worth working towards.

Tattomoosa commented 9 months ago

Was not familiar with Stormworks, very cool. I've done a lot of work towards in-editor previewing and put up a somewhat rough draft PR for feedback that implements it as well as adding an OceanSurface3D node which has subclasses to render to different meshes, as part of work towards eventually supporting Terrain3D. I've done this work along the lines of thinking what would have been useful for me in using this plugin before I dug into the code and understood more of it.

I've decided I agree with you about MotorVessel3D - that seems consistent with Godot's own CharacterController3D and Vehicle3D classes and I'm all for keeping consistency with the example Godot itself has set when possible. I think there's still a question about what behaviors MotorVessel3D should support in order to be a class that an end-user can extend into something functional relatively easily. Maybe it shouldn't do much more than provide buoyancy (as it does) and register propellers/rudders for a subclass to control the behavior of.

QuinnLeavitt commented 8 months ago

Are there any plans to have motorVessel3D have other methods of propulsion than propellers that have to be underwater?

I was thinking the movement seemed unrealistic for something like a sailing ship since the ship could still be caught in the wind even when it's out of the water. This could maybe just be something the user would toggle on or off in the export variables.