added constructors to MTLRegion #165

adamnemecek · 2020-07-30T22:37:58Z

No description provided.

chinedufn · 2020-07-30T23:38:08Z

src/types.rs

@@ -31,6 +31,37 @@ pub struct MTLRegion {
    pub size: MTLSize,
 }

+impl MTLRegion {
+    #[inline]


I'm curious to hear more about where you feel the line should be for abstractions around the raw metal APIs?

Mainly looking to better understand the motivation / thought process here.

More specifically, why should this land?

Cheers!!

Ah I see https://developer.apple.com/documentation/metal/1516011-mtlregionmake1d

Mind making names consistent with Metal and linking to the Metal docs as a doc comment for each function?

As far as naming, you need to strike a balance between the metal idioms and rust idioms. What do you suggest these be called? We already diverge from the make prefix e.g. new_render_command_encoder is called makeRenderCommandEncoder in metal.

I think one of the most important flows to support is

Person looks up Apple’s Metal Docs for how to do something

Person looks for the equivalent in metal-rs

Person finds what they need quickly

Given that goal (I’m just speaking for myself, you might think that’s a silly goal!) I’d say that the only changes we should make to function names or parameters is

snake casing because it’s more rusty and you’ll still be able to find what you’re looking for

More appropriate types such as accepting a &Path instead of an NSString when we need a file path.

As such, I’d personally lean towards calling these make_1d, make_2d and make_3d.

I’d say that the divergence in the render command encoder creator fits under the umbrella of #144

These are just my thoughts, will wait on @kvark

So these should be functions or constructors?

Ah sorry, I meant MTLRegion::make_1d etc constructors

I would still lean towards the "new" nomenclature since there isn't a single instance in metal-rs of a function starting with "make".

@kvark suggests renaming all functions (not just these but like in the whole framework) to "make" (pun intended) to be in line with metal. How about we merge this in and rename it with the rest if we decide to do that?

I just appreciate this PR in the first place so - I won’t stand in the way! That plan sounds fine to me.

Maybe just still add the doc comment linking to the corresponding Metal docs?

I think we should do that for all constructors/functions/methods/types/etc going forwards.

Oh and I still think these should at the very least be 1d 2d instead of d1 d2 ... but again ... I won’t stand in the way and bike shed this !

monocodus

This is an autogenerated code review.New suggestions: 3

The .monocodus config not found in your repo. Default config is used.
Check config documentation here

monocodus · 2020-07-31T02:20:24Z

examples/window/main.rs

@@ -112,6 +168,7 @@ fn main() {
                WindowEvent::CloseRequested => *control_flow = ControlFlow::Exit,
                WindowEvent::Resized(size) => {
                    layer.set_drawable_size(CGSize::new(size.width as f64, size.height as f64));
+


Suggested change

generated by rustfmt

monocodus · 2020-07-31T02:20:25Z

examples/window/main.rs

@@ -162,7 +219,15 @@ fn main() {

                let command_buffer = command_queue.new_command_buffer();
                let encoder = command_buffer.new_render_command_encoder(&render_pass_descriptor);
-                encoder.set_render_pipeline_state(&pipeline_state);
+
+                encoder.set_scissor_rect(MTLScissorRect { x: 0, y: 0, width: 100, height: 100});


Suggested change

encoder.set_scissor_rect(MTLScissorRect { x: 0, y: 0, width: 100, height: 100});

encoder.set_scissor_rect(MTLScissorRect {

x: 0,

y: 0,

width: 100,

height: 100,

});

generated by rustfmt

monocodus · 2020-07-31T02:20:25Z

examples/window/main.rs

+                encoder.set_vertex_buffer(0, Some(&clear_rect_buffer), 0);
+                encoder.draw_primitives_instanced(metal::MTLPrimitiveType::TriangleStrip, 0, 4, 1);
+
+                encoder.set_scissor_rect(MTLScissorRect { x: 0, y: 0, width: size.width as _, height: size.height as _});


Suggested change

encoder.set_scissor_rect(MTLScissorRect { x: 0, y: 0, width: size.width as _, height: size.height as _});

encoder.set_scissor_rect(MTLScissorRect {

x: 0,

y: 0,

width: size.width as _,

height: size.height as _,

});

generated by rustfmt

adamnemecek · 2020-07-31T02:26:44Z

Yeah I was actually just trying to rename that. The new PR is here #166.

…

On Thu, Jul 30, 2020 at 19:25 Chinedu Francis Nwafili < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/types.rs <#165 (comment)>: > @@ -31,6 +31,37 @@ pub struct MTLRegion { pub size: MTLSize, } +impl MTLRegion { + #[inline] Oh and I still think these should at the very least be 1d 2d instead of d1 d2 ... but again ... I won’t stand in the way and bike shed this ! — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#165 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABMRDZECAMJK2F77JGSDM3R6ITSRANCNFSM4PO2BI7Q> .

added constructors to MTLRegion

6bd3aba

monocodus bot approved these changes Jul 30, 2020

View reviewed changes

chinedufn reviewed Jul 30, 2020

View reviewed changes

adamnemecek added 3 commits July 30, 2020 17:50

added clear_rect rendering to example/window

d1c869d

cargo fmt

34900c6

scissoring

8806425

monocodus bot suggested changes Jul 31, 2020

View reviewed changes

adamnemecek closed this Jul 31, 2020

adamnemecek mentioned this pull request Jul 31, 2020

mtlregion constructors #166

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added constructors to MTLRegion #165

added constructors to MTLRegion #165

adamnemecek commented Jul 30, 2020

chinedufn Jul 30, 2020

chinedufn Jul 30, 2020 •

edited

Loading

adamnemecek Jul 30, 2020

chinedufn Jul 31, 2020

adamnemecek Jul 31, 2020

chinedufn Jul 31, 2020

adamnemecek Jul 31, 2020 •

edited

Loading

adamnemecek Jul 31, 2020 •

edited

Loading

chinedufn Jul 31, 2020

chinedufn Jul 31, 2020

monocodus bot left a comment

monocodus bot Jul 31, 2020

monocodus bot Jul 31, 2020

monocodus bot Jul 31, 2020

adamnemecek commented Jul 31, 2020 via email •

edited

Loading

added constructors to MTLRegion #165

added constructors to MTLRegion #165

Conversation

adamnemecek commented Jul 30, 2020

chinedufn Jul 30, 2020

Choose a reason for hiding this comment

chinedufn Jul 30, 2020 • edited Loading

Choose a reason for hiding this comment

adamnemecek Jul 30, 2020

Choose a reason for hiding this comment

chinedufn Jul 31, 2020

Choose a reason for hiding this comment

adamnemecek Jul 31, 2020

Choose a reason for hiding this comment

chinedufn Jul 31, 2020

Choose a reason for hiding this comment

adamnemecek Jul 31, 2020 • edited Loading

Choose a reason for hiding this comment

adamnemecek Jul 31, 2020 • edited Loading

Choose a reason for hiding this comment

chinedufn Jul 31, 2020

Choose a reason for hiding this comment

chinedufn Jul 31, 2020

Choose a reason for hiding this comment

monocodus bot left a comment

Choose a reason for hiding this comment

monocodus bot Jul 31, 2020

Choose a reason for hiding this comment

monocodus bot Jul 31, 2020

Choose a reason for hiding this comment

monocodus bot Jul 31, 2020

Choose a reason for hiding this comment

adamnemecek commented Jul 31, 2020 via email • edited Loading

chinedufn Jul 30, 2020 •

edited

Loading

adamnemecek Jul 31, 2020 •

edited

Loading

adamnemecek Jul 31, 2020 •

edited

Loading

adamnemecek commented Jul 31, 2020 via email •

edited

Loading