-
Notifications
You must be signed in to change notification settings - Fork 551
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CGS Update (part 2) #3034
CGS Update (part 2) #3034
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good, except when you want to draw an alpha rectangle. You cant do that since your directly putting it onto the canvas with no alpha blending.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your changes on CGS! Could you also add a few tests for your changes? :)
Okay its ready! |
nice work |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks nice! and works!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks veryyy nice
apparently we are having some technicall issues with json files and broom pulling epiczhul's stuff |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your work.
It would be great if you can add a docstring to all public methods you added but its not required.
source/Cosmos.Core/ACPI.cs
Outdated
using System.Runtime.InteropServices; | ||
|
||
namespace Cosmos.Core | ||
namespace MACPI.Cosmos.HAL.PCIE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we changing the namespace?
@@ -154,6 +154,20 @@ public unsafe void Copy(int aStart, int[] aData, int aIndex, int aCount) | |||
} | |||
} | |||
|
|||
public unsafe void Get(int aStart, int[] aData, int aIndex, int aCount) | |||
{ | |||
// TODO throw exception if aStart and aCount are not in bound. I've tried to do this but Bochs dies :-( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should no longer be an issue and can be checked
} | ||
|
||
int length = addr; | ||
ptr -= 4; | ||
Console.WriteLine("[ACPI] ACPI Initialized"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we wanted to get rid of non-user controllable WriteLines during boot
source/Cosmos.Core/PCIE/PCIDevice.cs
Outdated
using System; | ||
using Cosmos.Core; | ||
|
||
namespace Cosmos.HAL.PCIE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
namespace and directory dont match
|
||
PCIExpress.Initialize(); | ||
|
||
Console.Write("[PCI] PCI Initialized. "); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this writes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi, leaving them there is best practice because you will have feedback of the PCI and PCIE initialization
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we allow the user to enable this feedback when they want it but also give them a way to disable it?
using System.Runtime.InteropServices; | ||
using PCIDevice = Cosmos.HAL.PCIE.PCIDevice; | ||
|
||
namespace Cosmos.HAL.PCIE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
/// <param name="maxHeight">Max image height to display</param> | ||
public virtual void CroppedDrawImage(Image image, int x, int y, int maxWidth, int maxHeight, bool preventOffBoundPixels = true) | ||
{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this left empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't know tho i could fix it
@@ -11,25 +11,27 @@ | |||
"Microsoft.NETCore.Platforms": "1.1.0" | |||
} | |||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this dependency added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think i left a using from when i was testing it
hi, can you please not merge as of now, i am trying to fix some issues with compileing and also json and dependency stuff, my visual studio messed up the repo |
i am leaving dependencies and json to @SzymekkYT
@MishaProductions all of the json files have been fixed.... could you merge (sorry for ping) |
Uhh yeah i wanted to make a new PR without Samma's PCIe but i guess github said no |
Added
aaand PCIe by @Samma2009