[WIP, alpha] hal #463

#version 450 core
#extension GL_ARB_separate_shader_objects : enable

Is this something we should recommend for all shaders until there is a good shading solution (cross-API, eventually in Rust) available?

amethyst_renderer_new/Cargo.toml, line 15 at r7 (raw file):

use renderer::vertex::PosColor;

fn main() {

How much of this low-level code will the user need to set up a basic game in the final iteration?

Comments from Reviewable

torkleyy · Nov 29, 2017

Reviewed 4 of 17 files at r1, 5 of 18 files at r2, 2 of 20 files at r3, 1 of 6 files at r5, 2 of 24 files at r6.
Review status: 13 of 34 files reviewed at latest revision, 34 unresolved discussions, some commit checks failed.

readme = "README.md"
license = "MIT OR Apache-2.0"

Should be "MIT/Apache-2.0"

amethyst_renderer_new/Cargo.toml, line 27 at r7 (raw file):

[dependencies]
amethyst_core = { path = "../amethyst_core" }

Don't forget to specify versions here.

amethyst_renderer_new/rustfmt.toml, line 1 at r7 (raw file):

reorder_imported_names = true

Do we really need one config per crate?

amethyst_renderer_new/src/relevant.rs, line 2 at r7 (raw file):

/// Type that can't be silently dropped.

Types don't get dropped.

amethyst_renderer_new/src/relevant.rs, line 4 at r7 (raw file):

/// Type that can't be silently dropped.
/// If structure has field with type `Relevant`
/// than this structure can't be silently dropped either.

"than" should be "then", but I'd probably rewrite the sentence as "If a structure has a Disposable value, it can't be automatically dropped either."

amethyst_renderer_new/src/relevant.rs, line 12 at r7 (raw file):

///
#[derive(Clone, Debug, PartialOrd, PartialEq, Ord, Eq, Hash)]
pub struct Relevant;

Rename to Disposable?

amethyst_renderer_new/src/relevant.rs, line 23 at r7 (raw file):

impl Drop for Relevant {
    fn drop(&mut self) {
        panic!("This type can't be dropped")

This really isn't something I want the user to deal with, I hope we can keep this fully internal.

amethyst_renderer_new/src/utils.rs, line 4 at r7 (raw file):

#[inline(always)]
pub fn is_slice_sorted<T>(slice: &[T]) -> bool

This seems very expensive for what it's doing. I don't know where it's used, but if it's necessary I'd recommend to just check if x_n < x_n+1 holds for every n from 0 to len - 2.

amethyst_renderer_new/src/utils.rs, line 14 at r7 (raw file):

#[inline(always)]
pub fn is_slice_sorted_by_key<T, K, F>(slice: &[T], f: F) -> bool

Same as above.

#version 450 core
#extension GL_ARB_separate_shader_objects : enable

Is this something we should recommend for all shaders until there is a good shading solution (cross-API, eventually in Rust) available?

amethyst_renderer_new/src/utils.rs, line 4 at r7 (raw file):

use renderer::vertex::PosColor;

fn main() {

How much of this low-level code will the user need to set up a basic game in the final iteration?

Comments from Reviewable

So I've reviewed the easy files now so that I can concentrate on the other stuff. Is there some strategy you'd recommend for reviewing? Or some high-level overview that would make it easier?

FYI, I reviewed on https://reviewable.io, so it's probably easier to respond there.

torkleyy · Nov 29, 2017

So I've reviewed the easy files now so that I can concentrate on the other stuff. Is there some strategy you'd recommend for reviewing? Or some high-level overview that would make it easier?

FYI, I reviewed on https://reviewable.io, so it's probably easier to respond there.

Review status: 13 of 34 files reviewed at latest revision, 34 unresolved discussions, some commit checks failed.

Previously, torkleyy (Thomas Schaller) wrote…

This seems very expensive for what it's doing. I don't know where it's used, but if it's necessary I'd recommend to just check if x_n < x_n+1 holds for every n from 0 to len - 2.

This is very expensive of large N. But used only in debug_assert! for really small slices.
But I agree that it should be optimized to be O(n).

Previously, torkleyy (Thomas Schaller) wrote…

Is this something we should recommend for all shaders until there is a good shading solution (cross-API, eventually in Rust) available?

I have no opinion on that yet.

amethyst_renderer_new/src/utils.rs, line 4 at r7 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

How much of this low-level code will the user need to set up a basic game in the final iteration?

None.
The user will fill configuration structure (or even deserialize it) and call build.

Comments from Reviewable

omni-viral · Nov 29, 2017

Review status: 13 of 34 files reviewed at latest revision, 34 unresolved discussions, some commit checks failed.

Previously, torkleyy (Thomas Schaller) wrote…

This seems very expensive for what it's doing. I don't know where it's used, but if it's necessary I'd recommend to just check if x_n < x_n+1 holds for every n from 0 to len - 2.

This is very expensive of large N. But used only in debug_assert! for really small slices.
But I agree that it should be optimized to be O(n).

Previously, torkleyy (Thomas Schaller) wrote…

Is this something we should recommend for all shaders until there is a good shading solution (cross-API, eventually in Rust) available?

I have no opinion on that yet.

amethyst_renderer_new/src/relevant.rs, line 12 at r7 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

How much of this low-level code will the user need to set up a basic game in the final iteration?

None.
The user will fill configuration structure (or even deserialize it) and call build.

Comments from Reviewable

https://reviewable.io is so slow on my laptop.

omni-viral · Nov 29, 2017

https://reviewable.io is so slow on my laptop.

Review status: 13 of 34 files reviewed at latest revision, 34 unresolved discussions, some commit checks failed.

Previously, torkleyy (Thomas Schaller) wrote…

Rename to Disposable?

The name comes from here

amethyst_renderer_new/src/relevant.rs, line 23 at r7 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

This really isn't something I want the user to deal with, I hope we can keep this fully internal.

All higher-level types with Relevant values inside will do magic tricks in their Drop implementation to make user's life easier. But at cost in runtime.
For example Mesh can send Buffers back to the allocator over shared mpsc if user doesn't do that manually.
It adds some overhead but leaves user with a choice.

Comments from Reviewable

omni-viral · Nov 29, 2017

Review status: 13 of 34 files reviewed at latest revision, 34 unresolved discussions, some commit checks failed.

amethyst_renderer_new/src/relevant.rs, line 12 at r7 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Rename to Disposable?

The name comes from here

amethyst_renderer_new/src/relevant.rs, line 23 at r7 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

This really isn't something I want the user to deal with, I hope we can keep this fully internal.

All higher-level types with Relevant values inside will do magic tricks in their Drop implementation to make user's life easier. But at cost in runtime.
For example Mesh can send Buffers back to the allocator over shared mpsc if user doesn't do that manually.
It adds some overhead but leaves user with a choice.

Comments from Reviewable

@omni-viral It's slow for you? On my laptop it's faster than GH. For such a large PR you should just ensure you're using single-file mode.

torkleyy · Nov 29, 2017

@omni-viral It's slow for you? On my laptop it's faster than GH. For such a large PR you should just ensure you're using single-file mode.

Review status: 13 of 34 files reviewed at latest revision, 32 unresolved discussions, some commit checks failed.

amethyst_renderer_new/src/relevant.rs, line 12 at r7 (raw file):

Previously, omni-viral (Zakarum) wrote…

The name comes from here

At least the name isn't very obvious for me, but maybe it's more known to other people.

amethyst_renderer_new/src/graph/pass.rs, line 53 at r2 (raw file):

Previously, omni-viral (Zakarum) wrote…

It vice versa. I want all passes to be Debug to print messages inside the engine where I only have Box<AnyPass<B>>.
So AnyPass has to be Debug. But AnyPass is private trait. I can't bound types to it in public methods. Instead public methods has Pass bound and AnyPass is implemented for all Passes. But Pass needs Debug bound in order to implement AnyPass for all types that implements Pass.

Have no idea if someone could actually understand what I was trying to say here 😄

I did :)

Comments from Reviewable

torkleyy · Nov 29, 2017

Review status: 13 of 34 files reviewed at latest revision, 32 unresolved discussions, some commit checks failed.

amethyst_renderer_new/src/relevant.rs, line 12 at r7 (raw file):

Previously, omni-viral (Zakarum) wrote…

The name comes from here

At least the name isn't very obvious for me, but maybe it's more known to other people.

amethyst_renderer_new/src/graph/pass.rs, line 53 at r2 (raw file):

Previously, omni-viral (Zakarum) wrote…

It vice versa. I want all passes to be Debug to print messages inside the engine where I only have Box<AnyPass<B>>.
So AnyPass has to be Debug. But AnyPass is private trait. I can't bound types to it in public methods. Instead public methods has Pass bound and AnyPass is implemented for all Passes. But Pass needs Debug bound in order to implement AnyPass for all types that implements Pass.

Have no idea if someone could actually understand what I was trying to say here 😄

I did :)

Comments from Reviewable

Reviewed 1 of 18 files at r2, 3 of 20 files at r3, 10 of 24 files at r6, 8 of 12 files at r7.
Review status: 30 of 34 files reviewed at latest revision, 42 unresolved discussions, some commit checks failed.

amethyst_renderer_new/src/epoch.rs, line 9 at r7 (raw file):

use relevant::Relevant;

/// Epoch identifier.

Missing usage example / overview.

amethyst_renderer_new/src/epoch.rs, line 109 at r7 (raw file):

    /// Make all new `Ec` from this to be valid
    /// until specifyed `Epoch` expired

specified

amethyst_renderer_new/src/epoch.rs, line 159 at r7 (raw file):

}

pub struct DeletionQueue<T> {

Missing docs

amethyst_renderer_new/src/hal.rs, line 132 at r7 (raw file):

        adapter: Adapter<B>,
    ) -> (B::Device, Factory<B>, CommandCenter<B>) {
        println!("Try adapter: {:?}", adapter.info);

Yet again, we should really set up logging.

amethyst_renderer_new/src/mesh.rs, line 25 at r7 (raw file):

    errors {
        Incompatible {
            description("Incompatible"),

I don't think that this is any more descriptive than the defaults.

amethyst_renderer_new/src/mesh.rs, line 562 at r7 (raw file):

                vertex.0.push((&self.vbufs[index].buffer, 0));
                last = index;
                assert!(vertex_count == None || vertex_count == Some(self.vbufs[index].len));

nit: should be is_none

amethyst_renderer_new/src/mesh.rs, line 566 at r7 (raw file):

            } else {
                // Can't bind
                return Err(ErrorKind::Incompatible.into());

Can't you use bail! here? Or is that for string errors only?

amethyst_renderer_new/src/shaders.rs, line 247 at r7 (raw file):

    use std::fs::File;
    use std::io::Read;
    let mut file = File::open(path).chain_err(|| {

Only allowing shaders to be stored as a file is too restrictive.

amethyst_renderer_new/src/memory/mod.rs, line 160 at r7 (raw file):

    /// Returns tag value.
    /// This is unsafe as the caller must ensure that
    /// memory was freed

This is unclear.

amethyst_renderer_new/src/memory/mod.rs, line 273 at r7 (raw file):

    unsafe {
        forget(vec);
        Vec::from_raw_parts(p as *mut Y, (s * tsize) / ysize, (c * tsize) / ysize)

As far as I know, this is currently unspecified behavior in Rust. That means it works, but it's not guaranteed that it's supported by all Rust compilers in the future.

Comments from Reviewable

torkleyy · Nov 29, 2017

Reviewed 1 of 18 files at r2, 3 of 20 files at r3, 10 of 24 files at r6, 8 of 12 files at r7.
Review status: 30 of 34 files reviewed at latest revision, 42 unresolved discussions, some commit checks failed.

amethyst_renderer_new/src/epoch.rs, line 9 at r7 (raw file):

use relevant::Relevant;

/// Epoch identifier.

Missing usage example / overview.

amethyst_renderer_new/src/epoch.rs, line 109 at r7 (raw file):

    /// Make all new `Ec` from this to be valid
    /// until specifyed `Epoch` expired

specified

amethyst_renderer_new/src/epoch.rs, line 159 at r7 (raw file):

}

pub struct DeletionQueue<T> {

Missing docs

amethyst_renderer_new/src/hal.rs, line 132 at r7 (raw file):

        adapter: Adapter<B>,
    ) -> (B::Device, Factory<B>, CommandCenter<B>) {
        println!("Try adapter: {:?}", adapter.info);

Yet again, we should really set up logging.

amethyst_renderer_new/src/mesh.rs, line 25 at r7 (raw file):

    errors {
        Incompatible {
            description("Incompatible"),

I don't think that this is any more descriptive than the defaults.

amethyst_renderer_new/src/mesh.rs, line 562 at r7 (raw file):

                vertex.0.push((&self.vbufs[index].buffer, 0));
                last = index;
                assert!(vertex_count == None || vertex_count == Some(self.vbufs[index].len));

nit: should be is_none

amethyst_renderer_new/src/mesh.rs, line 566 at r7 (raw file):

            } else {
                // Can't bind
                return Err(ErrorKind::Incompatible.into());

Can't you use bail! here? Or is that for string errors only?

amethyst_renderer_new/src/shaders.rs, line 247 at r7 (raw file):

    use std::fs::File;
    use std::io::Read;
    let mut file = File::open(path).chain_err(|| {

Only allowing shaders to be stored as a file is too restrictive.

amethyst_renderer_new/src/memory/mod.rs, line 160 at r7 (raw file):

    /// Returns tag value.
    /// This is unsafe as the caller must ensure that
    /// memory was freed

This is unclear.

amethyst_renderer_new/src/memory/mod.rs, line 273 at r7 (raw file):

    unsafe {
        forget(vec);
        Vec::from_raw_parts(p as *mut Y, (s * tsize) / ysize, (c * tsize) / ysize)

As far as I know, this is currently unspecified behavior in Rust. That means it works, but it's not guaranteed that it's supported by all Rust compilers in the future.

Comments from Reviewable

@omni-viral What I've seen so far looks great, but it's lacking documentation both for important details and as overview.

torkleyy · Nov 29, 2017

@omni-viral What I've seen so far looks great, but it's lacking documentation both for important details and as overview.

lacking documentation

That's where I could use some help 😏

omni-viral · Nov 30, 2017

lacking documentation

That's where I could use some help 😏

Thing is my own overview isn't complete yet, but it's a requirement for providing a good review.

torkleyy · Nov 30, 2017

Thing is my own overview isn't complete yet, but it's a requirement for providing a good review.

Review status: 13 of 35 files reviewed at latest revision, 42 unresolved discussions, some commit checks failed.

amethyst_renderer_new/src/mesh.rs, line 25 at r7 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

I don't think that this is any more descriptive than the defaults.

There are defaults? 😄

amethyst_renderer_new/src/mesh.rs, line 566 at r7 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Can't you use bail! here? Or is that for string errors only?

Can. Thanks!

amethyst_renderer_new/src/graph/pass.rs, line 53 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

I did :)

Phew 😄

amethyst_renderer_new/src/hal/build.rs, line 132 at r7 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Yet again, we should really set up logging.

But not in scope of this PR.
If logging will be setup before this PR gets merged I'd replace print-logging with proper one.

amethyst_renderer_new/src/memory/mod.rs, line 273 at r7 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

As far as I know, this is currently unspecified behavior in Rust. That means it works, but it's not guaranteed that it's supported by all Rust compilers in the future.

I'm not sure how rustc can be changed so this will be broken.
If you are against this code you should also be against gfx_hal::memory::cast_slice.
Or your only concern is that it is Vec and it's totally OK for slices?
In that case I can replace Vec<T> with Box<[T]>

amethyst_renderer_new/rustfmt.toml, line 1 at r7 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Do we really need one config per crate?

Nope.

Comments from Reviewable

omni-viral · Nov 30, 2017

Review status: 13 of 35 files reviewed at latest revision, 42 unresolved discussions, some commit checks failed.

amethyst_renderer_new/src/mesh.rs, line 25 at r7 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

I don't think that this is any more descriptive than the defaults.

There are defaults? 😄

amethyst_renderer_new/src/mesh.rs, line 566 at r7 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Can't you use bail! here? Or is that for string errors only?

Can. Thanks!

amethyst_renderer_new/src/graph/pass.rs, line 53 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

I did :)

Phew 😄

amethyst_renderer_new/src/hal/build.rs, line 132 at r7 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Yet again, we should really set up logging.

But not in scope of this PR.
If logging will be setup before this PR gets merged I'd replace print-logging with proper one.

amethyst_renderer_new/src/memory/mod.rs, line 273 at r7 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

As far as I know, this is currently unspecified behavior in Rust. That means it works, but it's not guaranteed that it's supported by all Rust compilers in the future.

I'm not sure how rustc can be changed so this will be broken.
If you are against this code you should also be against gfx_hal::memory::cast_slice.
Or your only concern is that it is Vec and it's totally OK for slices?
In that case I can replace Vec<T> with Box<[T]>

amethyst_renderer_new/rustfmt.toml, line 1 at r7 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Do we really need one config per crate?

Nope.

Comments from Reviewable

amethyst_renderer_new/src/memory/mod.rs, line 273 at r7 (raw file):

Previously, omni-viral (Zakarum) wrote…

I'm not sure how rustc can be changed so this will be broken.
If you are against this code you should also be against gfx_hal::memory::cast_slice.
Or your only concern is that it is Vec and it's totally OK for slices?
In that case I can replace Vec<T> with Box<[T]>

At the very least the cast_pod_vec function should be marked as unsafe, as this could cause wild behavior. Not all representations of Pod types are valid representations of other Pod types. I actually think this specific line is probably OK though as vectors are guaranteed to form contiguous data. This is effectively a mass transmute though, which is very unsafe.

Comments from Reviewable

Xaeroxe · Nov 30, 2017

amethyst_renderer_new/src/memory/mod.rs, line 273 at r7 (raw file):

Previously, omni-viral (Zakarum) wrote…

I'm not sure how rustc can be changed so this will be broken.
If you are against this code you should also be against gfx_hal::memory::cast_slice.
Or your only concern is that it is Vec and it's totally OK for slices?
In that case I can replace Vec<T> with Box<[T]>

At the very least the cast_pod_vec function should be marked as unsafe, as this could cause wild behavior. Not all representations of Pod types are valid representations of other Pod types. I actually think this specific line is probably OK though as vectors are guaranteed to form contiguous data. This is effectively a mass transmute though, which is very unsafe.

Comments from Reviewable

Review status: 12 of 35 files reviewed at latest revision, 42 unresolved discussions, some commit checks failed.

amethyst_renderer_new/src/memory/mod.rs, line 273 at r7 (raw file):

Not all representations of Pod types are valid representations of other Pod types

T: Pod means that any bit sequence of appropriate size is valid T

Comments from Reviewable

omni-viral · Nov 30, 2017

Review status: 12 of 35 files reviewed at latest revision, 42 unresolved discussions, some commit checks failed.

amethyst_renderer_new/src/memory/mod.rs, line 273 at r7 (raw file):

Not all representations of Pod types are valid representations of other Pod types

T: Pod means that any bit sequence of appropriate size is valid T

Comments from Reviewable

Review status: 12 of 35 files reviewed at latest revision, 42 unresolved discussions, some commit checks failed.

amethyst_renderer_new/src/memory/mod.rs, line 273 at r7 (raw file):

Previously, omni-viral (Zakarum) wrote…

Not all representations of Pod types are valid representations of other Pod types

T: Pod means that any bit sequence of appropriate size is valid T

And this function ensures size and alignment restriction also.
That's why I consider it safe.

Comments from Reviewable

omni-viral · Nov 30, 2017

Review status: 12 of 35 files reviewed at latest revision, 42 unresolved discussions, some commit checks failed.

amethyst_renderer_new/src/memory/mod.rs, line 273 at r7 (raw file):

Previously, omni-viral (Zakarum) wrote…

Not all representations of Pod types are valid representations of other Pod types

T: Pod means that any bit sequence of appropriate size is valid T

And this function ensures size and alignment restriction also.
That's why I consider it safe.

Comments from Reviewable

Review status: 12 of 35 files reviewed at latest revision, 42 unresolved discussions, some commit checks failed.

amethyst_renderer_new/src/memory/mod.rs, line 273 at r7 (raw file):

you should also be against

I guess I am.

I'm not sure how rustc can be changed so this will be broken.

This is not limited to rustc: e.g. GCC Rust could force strict aliasing.

I actually think this specific line is probably OK

Sorry, I don't think that helps.

That's why I consider it safe.

You'd be surprised how much undefined behavior there is in languages like C and some of them are still there for unsafe Rust code. And while this code currently works, it's not something one should rely on because it's unspecified.

Comments from Reviewable

torkleyy · Nov 30, 2017

Review status: 12 of 35 files reviewed at latest revision, 42 unresolved discussions, some commit checks failed.

amethyst_renderer_new/src/memory/mod.rs, line 273 at r7 (raw file):

you should also be against

I guess I am.

I'm not sure how rustc can be changed so this will be broken.

This is not limited to rustc: e.g. GCC Rust could force strict aliasing.

I actually think this specific line is probably OK

Sorry, I don't think that helps.

That's why I consider it safe.

You'd be surprised how much undefined behavior there is in languages like C and some of them are still there for unsafe Rust code. And while this code currently works, it's not something one should rely on because it's unspecified.

Comments from Reviewable

Review status: 12 of 35 files reviewed at latest revision, 39 unresolved discussions, some commit checks failed.

amethyst_renderer_new/src/memory/mod.rs, line 273 at r7 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

you should also be against

I guess I am.

I'm not sure how rustc can be changed so this will be broken.

This is not limited to rustc: e.g. GCC Rust could force strict aliasing.

I actually think this specific line is probably OK

Sorry, I don't think that helps.

That's why I consider it safe.

You'd be surprised how much undefined behavior there is in languages like C and some of them are still there for unsafe Rust code. And while this code currently works, it's not something one should rely on because it's unspecified.

IIRC strict aliasing rules can't hurt you here.
As this function don't dereference pointers. And there is no aliasing before and after it.

gfx_hal::memory::cast_slice is OK with strict aliasing as it used shared references.
So you can't write to one slice and wait that it will affect another.
To see effects from strict aliasing optimizations you'll need convert & into &mut (under unsafe). And there is no wonder you'll have UB.

Comments from Reviewable

omni-viral · Nov 30, 2017

Review status: 12 of 35 files reviewed at latest revision, 39 unresolved discussions, some commit checks failed.

amethyst_renderer_new/src/memory/mod.rs, line 273 at r7 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

you should also be against

I guess I am.

I'm not sure how rustc can be changed so this will be broken.

This is not limited to rustc: e.g. GCC Rust could force strict aliasing.

I actually think this specific line is probably OK

Sorry, I don't think that helps.

That's why I consider it safe.

You'd be surprised how much undefined behavior there is in languages like C and some of them are still there for unsafe Rust code. And while this code currently works, it's not something one should rely on because it's unspecified.

IIRC strict aliasing rules can't hurt you here.
As this function don't dereference pointers. And there is no aliasing before and after it.

gfx_hal::memory::cast_slice is OK with strict aliasing as it used shared references.
So you can't write to one slice and wait that it will affect another.
To see effects from strict aliasing optimizations you'll need convert & into &mut (under unsafe). And there is no wonder you'll have UB.

Comments from Reviewable

Review status: 73 of 80 files reviewed at latest revision, 79 unresolved discussions, some commit checks failed.

amethyst_renderer/src/epoch.rs, line 129 at r13 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

"spcified"

Done.

amethyst_renderer/src/vertex.rs, line 357 at r15 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

missing tangent

Done.

amethyst_renderer/src/command/mod.rs, line 17 at r15 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

When can I have ComputeExecution? :)

Do you need it already? For now you can use GeneralExecution.

amethyst_renderer/src/command/mod.rs, line 93 at r15 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

span don't seem to exist in code below?

Outdated comments. Fixed.

amethyst_renderer/src/graph/build.rs, line 34 at r13 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

the type is called XXXView (single) but it contains a slice of views?

Done.

amethyst_renderer/src/graph/mod.rs, line 307 at r15 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

I'm not sure what singnal is but it sounds like a lot of fun! ;)

There were also some witches. But I drove them away.
I need to install some spellchecker.

amethyst_renderer/src/epoch.rs, line 129 at r13 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

We may want to supply some simple passes inside the actual renderer crate for the poor users :)

I think we can supply them in main crate.
But I have no strong opinion on this.

examples/hal/main.rs, line 1 at r15 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

Shouldn't this example be inside the renderer crate?

Actually it may be removed when our existing examples will use hal.

src/lib.rs, line 76 at r15 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

Hmm?

Temporary solution 😄
I will uncomment it back once those modules will compile.

Comments from Reviewable

omni-viral · Dec 12, 2017

Review status: 73 of 80 files reviewed at latest revision, 79 unresolved discussions, some commit checks failed.

Previously, kvark (Dzmitry Malyshau) wrote…

"spcified"

Done.

amethyst_renderer/src/vertex.rs, line 357 at r15 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

missing tangent

Done.

amethyst_renderer/src/command/mod.rs, line 17 at r15 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

When can I have ComputeExecution? :)

Do you need it already? For now you can use GeneralExecution.

amethyst_renderer/src/command/mod.rs, line 93 at r15 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

span don't seem to exist in code below?

Outdated comments. Fixed.

amethyst_renderer/src/graph/build.rs, line 34 at r13 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

the type is called XXXView (single) but it contains a slice of views?

Done.

amethyst_renderer/src/graph/mod.rs, line 307 at r15 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

I'm not sure what singnal is but it sounds like a lot of fun! ;)

There were also some witches. But I drove them away.
I need to install some spellchecker.

amethyst_renderer/src/mesh.rs, line 359 at r15 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

We may want to supply some simple passes inside the actual renderer crate for the poor users :)

I think we can supply them in main crate.
But I have no strong opinion on this.

examples/hal/main.rs, line 1 at r15 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

Shouldn't this example be inside the renderer crate?

Actually it may be removed when our existing examples will use hal.

src/lib.rs, line 76 at r15 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

Hmm?

Temporary solution 😄
I will uncomment it back once those modules will compile.

Comments from Reviewable

Review status: 73 of 80 files reviewed at latest revision, 73 unresolved discussions, some commit checks failed.

amethyst_renderer/src/cam.rs, line 87 at r15 (raw file):

Previously, omni-viral (Zakarum) wrote…

What the benefit?

Guarantee we use the same version for core::Transform and for renderer.

amethyst_renderer/src/lib.rs, line 24 at r15 (raw file):

Previously, omni-viral (Zakarum) wrote…

Either that or user will have to add backends as dependencies with correct versions.
If we will rexport only gfx_backend_XXX::Backend the user will have to import backend crate for doing platform specific stuff. Well, maybe the later is appropriate as if user write platform specific code he can import correct dependencies.

Yeah, I don't really understand the use case for having the backend crates reexported.

Previously, omni-viral (Zakarum) wrote…

It may become handy if the origins of the mesh and collision shape (or anything else with origin) will be different.

Possibly, I just don't see it used very often

amethyst_renderer/src/renderer.rs, line 30 at r15 (raw file):

Previously, omni-viral (Zakarum) wrote…

I's more like builder struct rather than config.

Hmm, ok. I just felt it should be kept close to the Window, because you can't pump events from winit without the Window.

amethyst_renderer/src/system.rs, line 1 at r15 (raw file):

Previously, omni-viral (Zakarum) wrote…

I'm thinking of moving input and windowing out of renderer responsibility.

Yeah, that would be optimal, but that also means we need to move winit::Window out from the renderer.

amethyst_renderer/src/texture.rs, line 1 at r15 (raw file):

Previously, omni-viral (Zakarum) wrote…

I'll add it to TODOs.
I think that with new API you can do skinning without placing matrices into texture.

Possibly.

amethyst_renderer/src/command/mod.rs, line 17 at r15 (raw file):

Previously, omni-viral (Zakarum) wrote…

Do you need it already? For now you can use GeneralExecution.

Na, it was just Compute is used further down :)

amethyst_renderer/src/mesh.rs, line 359 at r15 (raw file):

Previously, omni-viral (Zakarum) wrote…

I think we can supply them in main crate.
But I have no strong opinion on this.

Main crate should be kept to minimal imho.

Comments from Reviewable

Rhuagh · Dec 12, 2017

Review status: 73 of 80 files reviewed at latest revision, 73 unresolved discussions, some commit checks failed.

amethyst_renderer/src/cam.rs, line 87 at r15 (raw file):

Previously, omni-viral (Zakarum) wrote…

What the benefit?

Guarantee we use the same version for core::Transform and for renderer.

amethyst_renderer/src/lib.rs, line 24 at r15 (raw file):

Previously, omni-viral (Zakarum) wrote…

Either that or user will have to add backends as dependencies with correct versions.
If we will rexport only gfx_backend_XXX::Backend the user will have to import backend crate for doing platform specific stuff. Well, maybe the later is appropriate as if user write platform specific code he can import correct dependencies.

Yeah, I don't really understand the use case for having the backend crates reexported.

Previously, omni-viral (Zakarum) wrote…

It may become handy if the origins of the mesh and collision shape (or anything else with origin) will be different.

Possibly, I just don't see it used very often

amethyst_renderer/src/renderer.rs, line 30 at r15 (raw file):

Previously, omni-viral (Zakarum) wrote…

I's more like builder struct rather than config.

Hmm, ok. I just felt it should be kept close to the Window, because you can't pump events from winit without the Window.

amethyst_renderer/src/system.rs, line 1 at r15 (raw file):

Previously, omni-viral (Zakarum) wrote…

I'm thinking of moving input and windowing out of renderer responsibility.

Yeah, that would be optimal, but that also means we need to move winit::Window out from the renderer.

amethyst_renderer/src/texture.rs, line 1 at r15 (raw file):

Previously, omni-viral (Zakarum) wrote…

I'll add it to TODOs.
I think that with new API you can do skinning without placing matrices into texture.

Possibly.

amethyst_renderer/src/command/mod.rs, line 17 at r15 (raw file):

Previously, omni-viral (Zakarum) wrote…

Do you need it already? For now you can use GeneralExecution.

Na, it was just Compute is used further down :)

Previously, omni-viral (Zakarum) wrote…

I think we can supply them in main crate.
But I have no strong opinion on this.

Main crate should be kept to minimal imho.

Comments from Reviewable

Reviewed 1 of 33 files at r3, 5 of 39 files at r6, 2 of 31 files at r11, 7 of 107 files at r15, 7 of 36 files at r16.
Review status: 57 of 84 files reviewed at latest revision, 73 unresolved discussions, some commit checks failed.

Comments from Reviewable

Rhuagh · Dec 17, 2017

Reviewed 1 of 33 files at r3, 5 of 39 files at r6, 2 of 31 files at r11, 7 of 107 files at r15, 7 of 36 files at r16.
Review status: 57 of 84 files reviewed at latest revision, 73 unresolved discussions, some commit checks failed.

Comments from Reviewable

Reviewed 1 of 107 files at r15, 1 of 36 files at r16.
Review status: 58 of 84 files reviewed at latest revision, 73 unresolved discussions, some commit checks failed.

Comments from Reviewable

Rhuagh · Dec 17, 2017

Reviewed 1 of 107 files at r15, 1 of 36 files at r16.
Review status: 58 of 84 files reviewed at latest revision, 73 unresolved discussions, some commit checks failed.

Comments from Reviewable

Reviewed 1 of 64 files at r13, 2 of 107 files at r15, 11 of 36 files at r16.
Review status: 69 of 84 files reviewed at latest revision, 72 unresolved discussions, some commit checks failed.

Comments from Reviewable

Rhuagh · Dec 17, 2017

Reviewed 1 of 64 files at r13, 2 of 107 files at r15, 11 of 36 files at r16.
Review status: 69 of 84 files reviewed at latest revision, 72 unresolved discussions, some commit checks failed.

Comments from Reviewable

Reviewed 4 of 107 files at r15, 5 of 36 files at r16.
Review status: 74 of 84 files reviewed at latest revision, 72 unresolved discussions, some commit checks failed.

Comments from Reviewable

Rhuagh · Dec 17, 2017

Reviewed 4 of 107 files at r15, 5 of 36 files at r16.
Review status: 74 of 84 files reviewed at latest revision, 72 unresolved discussions, some commit checks failed.

Comments from Reviewable

Reviewed 1 of 107 files at r15, 2 of 36 files at r16.
Review status: 76 of 84 files reviewed at latest revision, 72 unresolved discussions, some commit checks failed.

Comments from Reviewable

Rhuagh · Dec 17, 2017

Reviewed 1 of 107 files at r15, 2 of 36 files at r16.
Review status: 76 of 84 files reviewed at latest revision, 72 unresolved discussions, some commit checks failed.

Comments from Reviewable

Reviewed 1 of 31 files at r11, 7 of 107 files at r15, 10 of 36 files at r16.
Review status: all files reviewed at latest revision, 71 unresolved discussions, some commit checks failed.

Comments from Reviewable

Rhuagh · Dec 17, 2017

Reviewed 1 of 31 files at r11, 7 of 107 files at r15, 10 of 36 files at r16.
Review status: all files reviewed at latest revision, 71 unresolved discussions, some commit checks failed.

Comments from Reviewable

Reviewed 3 of 39 files at r6, 2 of 31 files at r11, 7 of 107 files at r15, 11 of 11 files at r17.
Review status: all files reviewed at latest revision, 72 unresolved discussions.

name = "pong_tutorial_02"
path = "examples/pong_tutorial_02/main.rs"
name = "hal"

This looks broken.

Comments from Reviewable

Rhuagh · Dec 17, 2017

Reviewed 3 of 39 files at r6, 2 of 31 files at r11, 7 of 107 files at r15, 11 of 11 files at r17.
Review status: all files reviewed at latest revision, 72 unresolved discussions.

name = "pong_tutorial_02"
path = "examples/pong_tutorial_02/main.rs"
name = "hal"

This looks broken.

Comments from Reviewable

Review status: 84 of 85 files reviewed at latest revision, 72 unresolved discussions.

Previously, Rhuagh (Simon Rönnberg) wrote…

This looks broken.

Done.

Comments from Reviewable

omni-viral · Dec 17, 2017

Review status: 84 of 85 files reviewed at latest revision, 72 unresolved discussions.

amethyst_renderer/src/cirque.rs, line 32 at r19 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

This looks broken.

Done.

Comments from Reviewable

Reviewed 1 of 1 files at r18.
Review status: all files reviewed at latest revision, 71 unresolved discussions.

Comments from Reviewable

Rhuagh · Dec 19, 2017

Reviewed 1 of 1 files at r18.
Review status: all files reviewed at latest revision, 71 unresolved discussions.

Comments from Reviewable

Reviewed 2 of 31 files at r11, 10 of 108 files at r15, 2 of 37 files at r16, 18 of 18 files at r19.
Review status: all files reviewed at latest revision, 75 unresolved discussions.

    }

    pub fn get(&mut self, span: Range<Epoch>) -> Option<CirqueRef<T>> {

Some documentation would be good here, describing the invariants of this function. I'm unsure if the first check is correct.

amethyst_renderer/src/cirque.rs, line 49 at r19 (raw file):

    }

    pub fn get_or_try_insert<F, E>(&mut self, span: Range<Epoch>, mut f: F) -> Result<CirqueRef<T>, E>

Same as above.

amethyst_renderer/src/epoch.rs, line 84 at r19 (raw file):

or >= ?

amethyst_renderer/src/epoch.rs, line 138 at r19 (raw file):

    #[inline]
    pub unsafe fn get_unsafe<'a>(&self, until: Epoch) -> Option<&'a T> {
        if self.valid_until >= until {

This differs from the valid_until function above. Is that intentional?

Comments from Reviewable

Rhuagh · Dec 21, 2017

Reviewed 2 of 31 files at r11, 10 of 108 files at r15, 2 of 37 files at r16, 18 of 18 files at r19.
Review status: all files reviewed at latest revision, 75 unresolved discussions.

amethyst_renderer/src/cirque.rs, line 32 at r19 (raw file):

    }

    pub fn get(&mut self, span: Range<Epoch>) -> Option<CirqueRef<T>> {

Some documentation would be good here, describing the invariants of this function. I'm unsure if the first check is correct.

amethyst_renderer/src/cirque.rs, line 49 at r19 (raw file):

    }

    pub fn get_or_try_insert<F, E>(&mut self, span: Range<Epoch>, mut f: F) -> Result<CirqueRef<T>, E>

Same as above.

amethyst_renderer/src/epoch.rs, line 84 at r19 (raw file):

or >= ?

amethyst_renderer/src/epoch.rs, line 138 at r19 (raw file):

    #[inline]
    pub unsafe fn get_unsafe<'a>(&self, until: Epoch) -> Option<&'a T> {
        if self.valid_until >= until {

This differs from the valid_until function above. Is that intentional?

Comments from Reviewable

Reviewed 1 of 31 files at r11, 10 of 108 files at r15, 1 of 38 files at r16, 1 of 20 files at r19, 31 of 31 files at r20.
Review status: all files reviewed at latest revision, 74 unresolved discussions.

Comments from Reviewable

Rhuagh · Dec 29, 2017

Reviewed 1 of 31 files at r11, 10 of 108 files at r15, 1 of 38 files at r16, 1 of 20 files at r19, 31 of 31 files at r20.
Review status: all files reviewed at latest revision, 74 unresolved discussions.

Comments from Reviewable

Reviewed 2 of 33 files at r12, 1 of 108 files at r15, 3 of 31 files at r20.
Review status: all files reviewed at latest revision, 80 unresolved discussions.

Cargo.toml, line 46 at r20 (raw file):

[dev-dependencies]
#amethyst_gltf = { path = "amethyst_gltf", version = "0.1" }

Just marking as unresolved, so we don't forget to fix it.

amethyst_renderer/src/cirque.rs, line 7 at r20 (raw file):

[dev-dependencies]
#amethyst_gltf = { path = "amethyst_gltf", version = "0.1" }
error-chain = "0.11"

Not too related to this PR, but maybe we should be using failure?

#[derive(Debug)]
pub struct Cirque<T> {

Please add docs what this type is for.

amethyst_renderer/src/cirque.rs, line 18 at r20 (raw file):

    }

    pub fn create<I>(iter: I) -> Self

Please rename to from_iter.

amethyst_renderer/src/cirque.rs, line 35 at r20 (raw file):

        if self.values
            .front()
            .map(|&(until, _)| until >= span.start)

I also don't understand this check. Please add some comments or docs here.

amethyst_renderer/src/epoch.rs, line 167 at r15 (raw file):

Previously, omni-viral (Zakarum) wrote…

Nope. For Eh it should be T: Send + Sync. For Ec it's ok.

I agree, please add a Send bound.

amethyst_renderer/src/epoch.rs, line 92 at r20 (raw file):

#[derive(Debug)]
pub struct Ec<T> {
    ptr: *const T,

Cells should be invariant over T I think.

amethyst_renderer/src/epoch.rs, line 125 at r20 (raw file):

    /// Returns `Some` if `Ec` hasn't expired yet
    /// (`CurrentEpoch::now()` is "earlier" than `self.valid_until()`).
    /// Returns `None` otherwise.

Ah, finally some docs. Nice!

amethyst_renderer/src/epoch.rs, line 141 at r20 (raw file):

    /// Returns `None` otherwise.
    #[inline]
    pub unsafe fn get_span<'a>(&self, span: Range<Epoch>) -> Option<&'a T> {

This is marked as unsafe, thus needs to document what guarantees the user needs to uphold.

amethyst_renderer/src/epoch.rs, line 325 at r20 (raw file):

}

#[cfg(not(target_pointer_width = "64"))]

Is this a good idea? Can't we support 32-bit?

Comments from Reviewable

torkleyy · Dec 29, 2017

Reviewed 2 of 33 files at r12, 1 of 108 files at r15, 3 of 31 files at r20.
Review status: all files reviewed at latest revision, 80 unresolved discussions.

Cargo.toml, line 46 at r20 (raw file):

[dev-dependencies]
#amethyst_gltf = { path = "amethyst_gltf", version = "0.1" }

Just marking as unresolved, so we don't forget to fix it.

amethyst_renderer/src/cirque.rs, line 7 at r20 (raw file):

[dev-dependencies]
#amethyst_gltf = { path = "amethyst_gltf", version = "0.1" }
error-chain = "0.11"

Not too related to this PR, but maybe we should be using failure?

#[derive(Debug)]
pub struct Cirque<T> {

Please add docs what this type is for.

amethyst_renderer/src/cirque.rs, line 18 at r20 (raw file):

    }

    pub fn create<I>(iter: I) -> Self

Please rename to from_iter.

amethyst_renderer/src/cirque.rs, line 35 at r20 (raw file):

        if self.values
            .front()
            .map(|&(until, _)| until >= span.start)

I also don't understand this check. Please add some comments or docs here.

amethyst_renderer/src/epoch.rs, line 167 at r15 (raw file):

Previously, omni-viral (Zakarum) wrote…

Nope. For Eh it should be T: Send + Sync. For Ec it's ok.

I agree, please add a Send bound.

amethyst_renderer/src/epoch.rs, line 92 at r20 (raw file):

#[derive(Debug)]
pub struct Ec<T> {
    ptr: *const T,

Cells should be invariant over T I think.

amethyst_renderer/src/epoch.rs, line 125 at r20 (raw file):

    /// Returns `Some` if `Ec` hasn't expired yet
    /// (`CurrentEpoch::now()` is "earlier" than `self.valid_until()`).
    /// Returns `None` otherwise.

Ah, finally some docs. Nice!

amethyst_renderer/src/epoch.rs, line 141 at r20 (raw file):

    /// Returns `None` otherwise.
    #[inline]
    pub unsafe fn get_span<'a>(&self, span: Range<Epoch>) -> Option<&'a T> {

This is marked as unsafe, thus needs to document what guarantees the user needs to uphold.

amethyst_renderer/src/epoch.rs, line 325 at r20 (raw file):

}

#[cfg(not(target_pointer_width = "64"))]

Is this a good idea? Can't we support 32-bit?

Comments from Reviewable

Forget about that note about variance, I just realized it's immutable only, so it should be correct.

torkleyy · Dec 29, 2017

Forget about that note about variance, I just realized it's immutable only, so it should be correct.

Review status: all files reviewed at latest revision, 83 unresolved discussions.

amethyst_renderer/src/epoch.rs, line 212 at r20 (raw file):

    #[inline]
    fn dispose(self, current: &CurrentEpoch) -> Result<T, Self> {
        if self.valid_until.epoch() <= current.now() {

This does not require an atomic operation.

amethyst_renderer/src/epoch.rs, line 315 at r20 (raw file):

    fn advance_to(&self, other: Epoch) {
        let value = other.0 as usize;
        if self.0.load(Ordering::Relaxed) < value {

Err..we need to acquire a lock here if I'm not completely mistaken. This line can lead to severe memory bugs.

amethyst_renderer/src/epoch.rs, line 316 at r20 (raw file):

        let value = other.0 as usize;
        if self.0.load(Ordering::Relaxed) < value {
            self.0.store(value as usize, Ordering::Relaxed); // TODO: Use `fetch_add`.

This isn't correct, it can cause a race condition.

Comments from Reviewable

torkleyy · Dec 29, 2017

Review status: all files reviewed at latest revision, 83 unresolved discussions.

amethyst_renderer/src/epoch.rs, line 212 at r20 (raw file):

    #[inline]
    fn dispose(self, current: &CurrentEpoch) -> Result<T, Self> {
        if self.valid_until.epoch() <= current.now() {

This does not require an atomic operation.

amethyst_renderer/src/epoch.rs, line 315 at r20 (raw file):

    fn advance_to(&self, other: Epoch) {
        let value = other.0 as usize;
        if self.0.load(Ordering::Relaxed) < value {

Err..we need to acquire a lock here if I'm not completely mistaken. This line can lead to severe memory bugs.

amethyst_renderer/src/epoch.rs, line 316 at r20 (raw file):

        let value = other.0 as usize;
        if self.0.load(Ordering::Relaxed) < value {
            self.0.store(value as usize, Ordering::Relaxed); // TODO: Use `fetch_add`.

This isn't correct, it can cause a race condition.

Comments from Reviewable

Review status: all files reviewed at latest revision, 83 unresolved discussions.

amethyst_renderer/src/epoch.rs, line 84 at r19 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

or >= ?

valid_until() returns first epoch when value may be invalidated.
Therefore it must be bigger then specified.

amethyst_renderer/src/epoch.rs, line 138 at r19 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

This differs from the valid_until function above. Is that intentional?

Yeap.
valid_until is the point in time when it may already be invalid.
span.end is the point in time when it won't be in use.

amethyst_renderer/src/epoch.rs, line 125 at r20 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Ah, finally some docs. Nice!

=^_^=

amethyst_renderer/src/epoch.rs, line 325 at r20 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Is this a good idea? Can't we support 32-bit?

We should. But it requires AtomicU64 which is only available in nightly.

Comments from Reviewable

omni-viral · Dec 29, 2017

Review status: all files reviewed at latest revision, 83 unresolved discussions.

amethyst_renderer/src/epoch.rs, line 84 at r19 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

or >= ?

valid_until() returns first epoch when value may be invalidated.
Therefore it must be bigger then specified.

amethyst_renderer/src/epoch.rs, line 138 at r19 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

This differs from the valid_until function above. Is that intentional?

Yeap.
valid_until is the point in time when it may already be invalid.
span.end is the point in time when it won't be in use.

amethyst_renderer/src/epoch.rs, line 125 at r20 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Ah, finally some docs. Nice!

=^_^=

amethyst_renderer/src/epoch.rs, line 325 at r20 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Is this a good idea? Can't we support 32-bit?

We should. But it requires AtomicU64 which is only available in nightly.

Comments from Reviewable

Review status: all files reviewed at latest revision, 82 unresolved discussions.

amethyst_renderer/src/epoch.rs, line 138 at r19 (raw file):

Previously, omni-viral (Zakarum) wrote…

Yeap.
valid_until is the point in time when it may already be invalid.
span.end is the point in time when it won't be in use.

Ah that makes sense. Please add this to the documentation above.

amethyst_renderer/src/epoch.rs, line 325 at r20 (raw file):

Previously, omni-viral (Zakarum) wrote…

We should. But it requires AtomicU64 which is only available in nightly.

Just casting it to a u64 isn't an option? Using AtomicU64 can cause severe performance problems on 32-bit platforms.

Comments from Reviewable

torkleyy · Dec 29, 2017

Review status: all files reviewed at latest revision, 82 unresolved discussions.

amethyst_renderer/src/epoch.rs, line 138 at r19 (raw file):

Previously, omni-viral (Zakarum) wrote…

Yeap.
valid_until is the point in time when it may already be invalid.
span.end is the point in time when it won't be in use.

Ah that makes sense. Please add this to the documentation above.

amethyst_renderer/src/epoch.rs, line 325 at r20 (raw file):

Previously, omni-viral (Zakarum) wrote…

We should. But it requires AtomicU64 which is only available in nightly.

Just casting it to a u64 isn't an option? Using AtomicU64 can cause severe performance problems on 32-bit platforms.

Comments from Reviewable

Review status: 80 of 100 files reviewed at latest revision, 81 unresolved discussions.

amethyst_renderer/src/epoch.rs, line 138 at r19 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Not too related to this PR, but maybe we should be using failure?

Let's take this as a separate discussion.

Previously, torkleyy (Thomas Schaller) wrote…

Ah that makes sense. Please add this to the documentation above.

I agree, documentation is enough then

Comments from Reviewable

Rhuagh · Dec 29, 2017

Review status: 80 of 100 files reviewed at latest revision, 81 unresolved discussions.