Skip to content
This repository has been archived by the owner on Apr 18, 2022. It is now read-only.

[WIP, alpha] hal #463

Closed
wants to merge 1 commit into from
Closed

[WIP, alpha] hal #463

wants to merge 1 commit into from

Conversation

zakarumych
Copy link
Member

@zakarumych zakarumych commented Oct 27, 2017

#Roadmap.

  • Memory manager
  • Framegraph
  • Initialization
  • Queue orchestration
  • Vertices 2.0
  • Meshes
  • Textures
  • Uniform caches
  • Descriptor pools
  • Camera
  • Basic lighting
  • Flat pass
  • Shaded pass
  • PBR pass
  • Rending system. Halfway there

What's left:

  • Move some code to small PR to be merged earlier.
  • Replace semaphores with other synchronization methods.
  • Fix examples
  • Text rendering.

This change is Reviewable

Copy link
Member

@torkleyy torkleyy left a comment

Choose a reason for hiding this comment

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

A few comments, it's a bit hard to review since you can't see what the actual changes are. I'm gonna clone the branch myself later and create a diff so I can review it better.

repository = "https://github.com/amethyst/amethyst"

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

Choose a reason for hiding this comment

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

MIT/Apache-2.0

@@ -0,0 +1,39 @@

#[cfg(feature = "metal")]
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we at some point allow to dynamically choose the backend? I think for an actual game it's preferable to have one binary which supports all the backends.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll think about it


use back::*;

error_chain! {
Copy link
Member

Choose a reason for hiding this comment

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

👍 👍 I scanned the error_chain! source today and I think it's something I want to have in Amethyst.

}


/// This allocator is do dumb it can't even free memory
Copy link
Member

Choose a reason for hiding this comment

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

maybe "is so dumb"?

Copy link
Member Author

Choose a reason for hiding this comment

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

correct!

impl DumbAllocator {
pub fn new() -> Self {
DumbAllocator {
granularity: 268_435_456, // 256 MB
Copy link
Member

Choose a reason for hiding this comment

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

Maybe 256 * 1024 * 1024 is better here?

@@ -0,0 +1,2 @@
reorder_imported_names = true
take_source_hints = true
Copy link
Member

Choose a reason for hiding this comment

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

You can remove this file, the base config for the workspace is applied

@zakarumych
Copy link
Member Author

I'll make it easier to view changes.

categories = ["rendering", "rendering::engine"]

documentation = "https://www.amethyst.rs/doc/master/amethyst_renderer/"
documentation = "https://docs.rs/amethyst_renderer/0.1.0/amethyst_renderer/"
Copy link
Member

Choose a reason for hiding this comment

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

Is it a good thing to change this? Especially locking the version isn't a good thing IMO.

Copy link
Member Author

Choose a reason for hiding this comment

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

Old link leads to 404

Copy link
Member Author

Choose a reason for hiding this comment

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

I think https://docs.rs/amethyst_renderer will be ideal. It will redirect to newest version automatically

Copy link
Member

Choose a reason for hiding this comment

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

I think the main crate has the correct link


pub mod error;
pub mod cam;
pub mod mesh;
Copy link
Member

Choose a reason for hiding this comment

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

Why are you removing all my imports? Could we please leave the crate structure flat?

Copy link
Member Author

Choose a reason for hiding this comment

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

This file was written from scratch. I'll add everything required later.

Copy link
Member

Choose a reason for hiding this comment

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

Okay. Please just make the modules private and reexport everything if that's possible.

}
}

impl From<OutOfMemory> for Error {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this use links in the error-chain macro?

Copy link
Member Author

Choose a reason for hiding this comment

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

OutOfMemory isn't Error
links are for other error-chains. For non-chain errors there is foreign_links. But it still required to implement Error

Copy link
Member

Choose a reason for hiding this comment

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

And OutOfMemory doesn't implement Error?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeap

@torkleyy
Copy link
Member

It would be really nice if you could just change what's absolutely necessary, because I don't want to end up with a renderer rewrite 2.0 that rewrites the whole engine (and blocks everything else). If there are things you need to change which don't have anything to do with the internals of the renderer crate, please create another PR if possible.

@Rhuagh
Copy link
Member

Rhuagh commented Oct 28, 2017

I think the renderer is gonna be mostly rewritten, because hal is such a big change it's kind of required.

@torkleyy
Copy link
Member

True, but if you had seen the previous renderer rewrite you'd know what I mean. It did change the whole crate, not just the renderer.

@Rhuagh
Copy link
Member

Rhuagh commented Oct 28, 2017

Oh I saw, and shuddered :)

@zakarumych
Copy link
Member Author

@torkleyy I try to make render API to look mostly the same.
And I'm going to simplify things where possible 😄
All sophisticated unsafe magic will be hidden inside renderer.

@Xaeroxe
Copy link
Member

Xaeroxe commented Oct 28, 2017

This one probably won't be as bad because of how well we've separated out our crates. The last renderer rewrite was functionally a rewrite of the whole project, this one isn't.

@Aceeri Aceeri added team: rendering status: working type: improvement An improvement or change to an existing feature. labels Oct 31, 2017
@torkleyy
Copy link
Member

I'm really happy to see this PR making progress, keep up the good work @omni-viral!

@torkleyy
Copy link
Member

THIS IS AWESOME!!

@torkleyy
Copy link
Member

I'm very confident I can review this in December, in case you didn't already merge it then.

Copy link
Member

@Xaeroxe Xaeroxe left a comment

Choose a reason for hiding this comment

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

I like it a lot! Much simplified. A large PR mandates a large review, so here's 19 review comments for you :P

//! Pass tells renderer how to convert inputs to image.
//!
//! # Definition
//! Pass - is a box which get some:
Copy link
Member

Choose a reason for hiding this comment

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

Might be better to use:

Pass is a trait that takes these inputs
...
...
and provides these outputs:
...
...

//! * Push constants
//! * Outputs
//! * Attachments
//! * Results of queries (Let's forget about it for now)
Copy link
Member

Choose a reason for hiding this comment

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

Forget about what? I'm not clear what's being said here.

Copy link
Member Author

Choose a reason for hiding this comment

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

About queries

//!
//! But it gets executed on the GPU.
//! So instead of writing it as a function and calling with arguments needed
//! we have to record commands into buffers and send them to the GPU.
Copy link
Member

Choose a reason for hiding this comment

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

This isn't 100% true as some passes like UI are pretty CPU driven (out of necessity). I think a more accurate way to describe this might be

The pass prepares and sends data to GPU shader buffers, the shaders will then process the data.

Copy link
Member Author

Choose a reason for hiding this comment

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

Shaders are only part of pipeline.

//! So instead of writing it as a function and calling with arguments needed
//! we have to record commands into buffers and send them to the GPU.
//!
//! We want a way to define a box which will record all necessarry commands in declarative fasion.
Copy link
Member

Choose a reason for hiding this comment

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

*fashion.

//!
//! We want a way to define a box which will record all necessarry commands in declarative fasion.
//! In order to feed this box with data we also need define `World -> [Input]` conversion
//! (in declarative fasion where possible).
Copy link
Member

Choose a reason for hiding this comment

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

*fashion


use back::*;

pub enum StagingBuffer {
Copy link
Member

Choose a reason for hiding this comment

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

What's this for? The line bringing this module in is commented out so should we just get rid of this file?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's under construction 😄

}
unsafe impl Pod for Color {}
impl Attribute for Color {
const NAME: &'static str = "1_color";
Copy link
Member

Choose a reason for hiding this comment

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

why not "color"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Attribute values (name + format) should be in strict order. Forcing it with name prefix was fast fix. It'll go away.

Copy link
Member Author

Choose a reason for hiding this comment

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

The order is Position -> Color -> Normal -> Tangent -> TexCoord.
Buffers can only contain those attributes in sorted order. And Mesh has to contain Buffers in sorted order. Why? To bind mesh for O(n + k) insted of O(n*k).

Copy link
Member

Choose a reason for hiding this comment

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

Then maybe it would be better to have a separate constant for the order?

Copy link
Member Author

Choose a reason for hiding this comment

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

@torkleyy Correct. Prefixing names is for fast-fixing

}
unsafe impl Pod for Normal {}
impl Attribute for Normal {
const NAME: &'static str = "2_normal";
Copy link
Member

Choose a reason for hiding this comment

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

why not "normal"?

}
unsafe impl Pod for Tangent {}
impl Attribute for Tangent {
const NAME: &'static str = "3_tangent";
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

}
unsafe impl Pod for TexCoord {}
impl Attribute for TexCoord {
const NAME: &'static str = "4_tex_coord";
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

/// Reflects [`Pass::draw`] function
///
/// [`Pass::draw`]: trait.Pass.html#tymethod.draw
fn draw<'a>(&mut self, cbuf: &mut B::CommandBuffer, world: &'a World);
Copy link
Member

Choose a reason for hiding this comment

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

Why have a lifetime here?

Copy link
Member Author

Choose a reason for hiding this comment

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

No reason

@kvark kvark self-assigned this Nov 26, 2017
@zakarumych zakarumych changed the title [WIP, early, pre-alpha] hal [WIP, alpha] hal Nov 28, 2017
Copy link
Contributor

@msiglreith msiglreith left a comment

Choose a reason for hiding this comment

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

Wheew, that's quite a large PR. Just a few notes from my side regarding some internal stuff. I haven't looked trough the whole code.

swapchain.present(queue, &presents);

// Wait defice to finish the work
device.wait_for_fences(&[finish], WaitFor::All, !0);
Copy link
Contributor

Choose a reason for hiding this comment

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

There should be multiple fences to reduce waiting time on the GPU. Currently it's CPU -> (submit) -> GPU -> (wait) -> CPU. This requires multiple command buffers per frame, etc. The main point is to reduce the time the CPU waits on the GPU to finish processing the currently requested frame. That's a pretty crucial design point imo to achieve good performance.

Copy link
Member Author

Choose a reason for hiding this comment

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

What pattern of using fences I should follow?
I was thinking of waiting for fence before rendering next frame instead of after.
I'm not sure if starting rendering next frame before previous is complete is a good idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

gpu
I made a small figure which visualizes the issue, I think. The current implementation introduces a stall between GPU-CPU due to the wait for the fence. The general proposal is to buffer a few frames (basically following double/tripple buffering of the swapchain) to give the GPU time to catch up and reducing the waiting time on the fence. In the figure I used frame3 waits unitl frame0 has finished on the GPU and can then re-use data (in particular, command pools, query buffers, etc.) of frame0. The drawback here is that this makes synchronization a bit trickier as fences ensure that objects can be safely accessed again. So, each frame in the chain requires an own command pool for example.

}
}

pub fn build(
Copy link
Contributor

Choose a reason for hiding this comment

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

The definition of a pass seems quite focused on Vulkan's renderpass concept. If I understand it correctly a pass is defined here as one renderpass with one subpass and pipeline. I would advise a lower level implementation which focuses purely on scheduling tasks and reducing the amount of synchronization handling for users writing rendering code. It might be tricky here to extend the current implementation to support compute and further optimizations.

Copy link
Member Author

Choose a reason for hiding this comment

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

The concept of Pass is quite abstract here. It's a function-like thing that can have Behavior-like (from FRP) arguments (attachments and buffers) and return Behavior-like result.
What you see in this method is mapping of general concept to RenderPasses.
It's tricky to implement this so I started with simples unoptimized solution. I'll try to make it faster in next iterations.

let descriptor_set_layout = device.create_descriptor_set_layout(pass.bindings());

// Create `PipelineLayout` from single `DescriptorSetLayout`
let pipeline_layout = device.create_pipeline_layout(&[&descriptor_set_layout]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Pipeline layouts and pipelines are not necessarily strongly-coupled. A pipeline layout can be compatible with multiple pipelines. Switching (non-compatible) pipeline layouts introduces costs due to rebinding descriptor sets and push constants.

Copy link
Member Author

Choose a reason for hiding this comment

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

But how often different pipelines can share pipeline layout?
Doesn't pipeline layout defines bindigs interface of shaders?
How often different shaders share bindings interface?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it defines the interface but can be a superset (some sort of 'uberlayout'). There is always a tradeoff between both approaches: minimal pipeline layout vs 'uberlayout'. Which to choose depends on the available pipelines. It might be interesting when having a lot of similar materials with lightly varying pipelines. I would say that expecially latest gen architectures can live with only a few pipeline layouts by using large descriptor sets, only bound once, but I'm not an expert.

Copy link
Member Author

Choose a reason for hiding this comment

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

This makes sense but I'd like to hide such details from user. So I need to magically create super-layout for few consecutive passes.

@torkleyy
Copy link
Member

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.


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

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.


examples/hal/hal.vert, line 2 at r7 (raw file):

#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?


examples/hal/main.rs, line 37 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
Copy link
Member

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
Copy link
Member

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.


Cargo.toml, line 47 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?


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

#[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
Copy link
Member

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

@torkleyy
Copy link
Member

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

@zakarumych
Copy link
Member Author

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

@torkleyy
Copy link
Member

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

@Rhuagh
Copy link
Member

Rhuagh commented Dec 29, 2017

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


Cargo.toml, line 47 at r20 (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.


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

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
Copy link
Member

Rhuagh commented Dec 29, 2017

Reviewed 5 of 108 files at r15, 2 of 31 files at r20, 18 of 20 files at r21.
Review status: 98 of 100 files reviewed at latest revision, 83 unresolved discussions.


amethyst_animation/Cargo.toml, line 25 at r21 (raw file):

minterpolate = "0.2"
serde = { version = "1.0", features = ["derive"] }
specs = { version = "0.10", git = "https://github.com/slide-rs/specs.git" }

Why use the git version?


amethyst_renderer/src/graph/pass.rs, line 114 at r21 (raw file):

    ///
    /// * binding `DescriptorSet`s
    /// * recording `Graphics` commands to `CommandBuffer`

These docs are identical to draw_inline below, that doesn't seem right?


Comments from Reviewable

@Rhuagh
Copy link
Member

Rhuagh commented Dec 29, 2017

Reviewed 2 of 108 files at r15, 2 of 20 files at r21.
Review status: all files reviewed at latest revision, 83 unresolved discussions.


Comments from Reviewable

@zakarumych
Copy link
Member Author

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


amethyst_animation/Cargo.toml, line 25 at r21 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

Why use the git version?

Entry API.


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

Previously, torkleyy (Thomas Schaller) wrote…

This does not require an atomic operation.

This one doesn't


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

Previously, torkleyy (Thomas Schaller) wrote…

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

The whole point of using atomics here is to not acquire locks.
Current code works because all parallel advance_to are called with same other argument.
I don't know how to guarantee it yet.


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

Previously, torkleyy (Thomas Schaller) wrote…

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

It can in general.


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

Previously, torkleyy (Thomas Schaller) wrote…

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

No. We need to store u64 in counter so that it wouldn't overflow in any reasonable time.
u32 will overflow in days.
u64 won't overflow in millennia.
And we need to perform atomic operations on it.


Comments from Reviewable

@torkleyy
Copy link
Member

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


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

Previously, omni-viral (Zakarum) wrote…

The whole point of using atomics here is to not acquire locks.
Current code works because all parallel advance_to are called with same other argument.
I don't know how to guarantee it yet.

With locks I was talking about atomics, actually.


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

Previously, omni-viral (Zakarum) wrote…

It can in general.

This doesn't establish any sort of happens-before relation, so how and why do you think it works?


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

Previously, omni-viral (Zakarum) wrote…

No. We need to store u64 in counter so that it wouldn't overflow in any reasonable time.
u32 will overflow in days.
u64 won't overflow in millennia.
And we need to perform atomic operations on it.

Yeah, I know. Still, it's an alternative.


Comments from Reviewable

@zakarumych
Copy link
Member Author

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


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

Previously, torkleyy (Thomas Schaller) wrote…

This doesn't establish any sort of happens-before relation, so how and why do you think it works?

Because Relaxed (Monotonic in LLVM) is enough here.
This atomic operations doesn't need to protect any other memory operation.


Comments from Reviewable

@Rhuagh
Copy link
Member

Rhuagh commented Jan 10, 2018

Reviewed 1 of 31 files at r11, 1 of 33 files at r12, 10 of 108 files at r15, 1 of 20 files at r19, 2 of 31 files at r20, 22 of 22 files at r22.
Review status: all files reviewed at latest revision, 83 unresolved discussions.


amethyst_renderer/src/cirque.rs, line 66 at r22 (raw file):

    /// Get constant entry for `span`.
    /// If queue is not empty - marks the last item in queue to be in use until `span.end`
    /// and returns mutable reference to it.

I think this should say immutable?


Comments from Reviewable

@Rhuagh
Copy link
Member

Rhuagh commented Jan 17, 2018

Reviewed 5 of 39 files at r6, 3 of 31 files at r11, 14 of 108 files at r15, 82 of 82 files at r23.
Review status: all files reviewed at latest revision, 82 unresolved discussions.


Comments from Reviewable

@Xaeroxe
Copy link
Member

Xaeroxe commented Jan 18, 2018

@omni-viral Looks like you have a lot of commits in here merged from prior PRs. Looks like this could use a rebase.

@zakarumych
Copy link
Member Author

zakarumych commented Jan 18, 2018

@Xaeroxe indeed. But they aren't merged. I rebased to "develop". But this PR is created against "hal-integration"

@torkleyy
Copy link
Member

Reviewed 2 of 83 files at r23, 2 of 39 files at r24, 2 of 21 files at r25.
Review status: 109 of 146 files reviewed at latest revision, 86 unresolved discussions.


amethyst_core/Cargo.toml, line 21 at r25 (raw file):

error-chain = "0.11"
fnv = "1.0"
hibitset = { version = "0.4.1" }

No need for an inline table.x


amethyst_core/src/events_pump.rs, line 3 at r25 (raw file):

use shrev::EventChannel;

Does this file belong into this PR? As I've requested before, could you please extract some parts from your PR and create a separate PR that we can merge right now? That would help reviewers of this PR by reducing the number of files and you don't have to rebase if we change files.


amethyst_renderer/Cargo.toml, line 33 at r24 (raw file):

fnv = "1.0"
gfx-hal = { version = "0.1.0", features = ["mint", "serde"], path = "/Users/zakarum/pet/gfx/src/hal" }
hibitset = { version = "0.4.1" }

No need for an inline table anymore.xxx


amethyst_renderer/Cargo.toml, line 43 at r24 (raw file):

shred = "0.5"
specs = { version = "0.10", git = "https://github.com/slide-rs/specs.git" }
thread_profiler = { version = "0.1" }

No need for an inline table.


amethyst_renderer/src/camera.rs, line 96 at r25 (raw file):

}

impl From<Camera> for [[f32; 4]; 4] {

Why create a From implementation instead of a method?


Comments from Reviewable

@zakarumych zakarumych changed the base branch from feature/hal-integration to develop January 18, 2018 17:10
@Rhuagh
Copy link
Member

Rhuagh commented Jan 19, 2018

Review status: 38 of 99 files reviewed at latest revision, 86 unresolved discussions, some commit checks failed.


amethyst_core/src/events_pump.rs, line 3 at r25 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Does this file belong into this PR? As I've requested before, could you please extract some parts from your PR and create a separate PR that we can merge right now? That would help reviewers of this PR by reducing the number of files and you don't have to rebase if we change files.

That would require quite a bit of restructuring of the old renderer unfortunately


Comments from Reviewable

@Rhuagh
Copy link
Member

Rhuagh commented Jan 19, 2018

Reviewed 5 of 40 files at r6, 3 of 31 files at r11, 2 of 67 files at r13, 22 of 111 files at r15, 1 of 22 files at r19, 2 of 32 files at r20, 11 of 39 files at r24, 6 of 21 files at r25, 52 of 52 files at r26.
Review status: all files reviewed at latest revision, 87 unresolved discussions, some commit checks failed.


amethyst_renderer/src/upload.rs, line 53 at r26 (raw file):

        offset: u64,
        data: D,
    ) -> Result<Self, ::failure::Error>

If possible, can we use imports instead?


Comments from Reviewable

@zakarumych
Copy link
Member Author

Review status: all files reviewed at latest revision, 87 unresolved discussions, some commit checks failed.


amethyst_renderer/src/upload.rs, line 53 at r26 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

If possible, can we use imports instead?

I'll try 😄


Comments from Reviewable

@zakarumych
Copy link
Member Author

Review status: all files reviewed at latest revision, 87 unresolved discussions, some commit checks failed.


amethyst_core/src/events_pump.rs, line 3 at r25 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

That would require quite a bit of restructuring of the old renderer unfortunately

You can just not use it with old renderer 😸


Comments from Reviewable

@Rhuagh
Copy link
Member

Rhuagh commented Jan 19, 2018

Review status: all files reviewed at latest revision, 87 unresolved discussions, some commit checks failed.


amethyst_core/src/events_pump.rs, line 3 at r25 (raw file):

Previously, omni-viral (Zakarum) wrote…

You can just not use it with old renderer 😸

Or we use it from the inside the old renderer.


Comments from Reviewable

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
team: rendering type: improvement An improvement or change to an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants