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

[WIP] Transform Refactor #1334

Open
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@jojolepro
Copy link
Member

jojolepro commented Jan 9, 2019

As I discussed earlier, I made some pretty significant changes to how Transform and GlobalTransform work.

The fork is located here: https://github.com/jojolepro/amethyst/tree/transform

Here's a quick recap:

  • GlobalTransform has been removed, and the fields were moved into Transform
  • Transform is now generic over the N: Real bound (f32, f64).

Here's what is left to do:

  • Fix the broken code
  • Fix the broken docs

Here's some numbers:

  • There is 92 mentions of "GlobalTransform" in the whole codebase (includes book).
  • There is 840 mentions of "Transform".

Here are the crates that will need to be adapted to support this change and tested. I would suggest picking between two and four of them for each person wanting to participate.

When you are in charge of a crate, your job is to make it compile, along with the tests, and adapt the documentation where applicable.

Thanks!

  • amethyst_animation
  • amethyst_config
  • amethyst_derive
  • amethyst_locale
  • amethyst_ui
  • amethyst_assets
  • amethyst_controls
  • amethyst_gltf
  • amethyst_network
  • amethyst_utils
  • amethyst_audio
  • amethyst_core
  • amethyst_input
  • amethyst_renderer
  • book
@@ -38,25 +33,14 @@ impl<'a> System<'a> for TransformSystem {
type SystemData = (
Entities<'a>,
ReadExpect<'a, ParentHierarchy>,
ReadStorage<'a, Transform>,
WriteStorage<'a, Transform3<f32>>,

This comment has been minimized.

@Frizi

Frizi Jan 9, 2019

Member

If systems are just going to hardcode the generic, what's the utility of having it generic at all? I see many potential problems of different systems trying to use either f32 of f64, which will end up being a totally separate components.

This comment has been minimized.

@Frizi

Frizi Jan 9, 2019

Member

We would need to propagate the generic with bounds to every single system that uses transforms (which will probably be a lot). It will add a lot of noise and possible source of bugs, yet I'm not sure what are the upsides to counter that.

This comment has been minimized.

@jojolepro

jojolepro Jan 9, 2019

Author Member

that's me being dumb, I forgot to update it correctly.
What I'm trying to go for is forcing all systems to have this bound:

N: Real = f32

That way you don't need additional code if you want the default f32, and you can still enable f64 easily when creating the systems.

This comment has been minimized.

@Frizi

Frizi Jan 9, 2019

Member

You will have to remember to register the right component system types to the ECS though. It's not really obvious to newcommers that all of those types must match, otherwise things will silently fail to see each other.

This comment has been minimized.

@jojolepro

jojolepro Jan 9, 2019

Author Member

Except with documentation, I don't see how we could ensure this.

I don't think there's a way to enforce them to all be the same at the type level.

This comment has been minimized.

@Frizi

Frizi Jan 9, 2019

Member

There isn't. That's the point. You might also actually have both at the same time if you so desire. I'm just in general not sure what's the point of having support for f64 at all. I might be not up to date. Were there any usecases presented for that?

I fear that this will push people into unnecessarily implementing their own systems in a generic way. Just because hardcoding the type might feel like a tech debt, people will often do it. But in reality it might be totally unnecessary to care about generics here at all. This problem might probably be solved by good documentation. It just feels like a bunch of unnecessary noise to me anyway.

This comment has been minimized.

@Frizi

Frizi Jan 9, 2019

Member

Also note that this might have an unitentional implact on other teams down the road. I suspect that this Transform component will be used to position things in the editor for example. If you want to be oblivious to the chosen type, you have to somehow support both. What if an entity has both at the same time? Also any other components that end up being often combined with Transform might end up needing the same generic in order to effectively do math between them without precision problems. It is just very "infectious".

Another example is the system I'm currently working on - encoders. I would have to implement the encoder in generic way and explain that the registration needs the correct Real type in order to take any effect. Now supporting Transform<f64> actually requires changes to the shader source code - all my matrices must use double precision. This quickly gets out of hands.

This comment has been minimized.

@kabergstrom

kabergstrom Jan 10, 2019

I haven't read through everything here yet, just wanted to chime in on the aspect of supporting f64. Star Citizen's team went as far as to say

64-bit world space is one of the driving elements behind making Star Citizen possible.

Source

I don't think the built-in encoders need to explicitly support f64 though.

This comment has been minimized.

@Rhuagh

Rhuagh Jan 14, 2019

Member

As an example, I would never run 3d physics on f32 personally, because the loss of information can lead to stability issues.

@CBenoit

This comment has been minimized.

Copy link
Contributor

CBenoit commented Feb 15, 2019

If it's okay, I'm willing to help. Maybe two crates as I'm not very experimented with amethyst internals.

@jojolepro

This comment has been minimized.

Copy link
Member Author

jojolepro commented Feb 15, 2019

Cool! Feel free to pick those you feel the most comfortable with :)

@CBenoit

This comment has been minimized.

Copy link
Contributor

CBenoit commented Feb 15, 2019

I'll start with amethyst_network then!

@jojolepro

This comment has been minimized.

Copy link
Member Author

jojolepro commented Feb 16, 2019

Does amethyst network even use transforms at all? :)

@CBenoit

This comment has been minimized.

Copy link
Contributor

CBenoit commented Feb 16, 2019

Check! Not even a single one. 🤣 I'll do amethyst_animation instead!

@jojolepro

This comment has been minimized.

Copy link
Member Author

jojolepro commented Feb 16, 2019

amethyst_core now compiles (excluding tests). That way, it will be easier to fix the other crates. ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment